Skip to content
This repository was archived by the owner on Feb 5, 2024. It is now read-only.

Attribute access for partially split objects#27

Merged
KPrasch merged 7 commits intonucypher:masterfrom
jMyles:master
Apr 13, 2020
Merged

Attribute access for partially split objects#27
KPrasch merged 7 commits intonucypher:masterfrom
jMyles:master

Conversation

@jMyles
Copy link
Contributor

@jMyles jMyles commented Apr 9, 2020

Fixes #26

thing3=(int, 2, {"byteorder": "big"})
)
def sip(self):
return "Mmmm"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ELAINE: Yeah, in college I sat next to an Alex in art history. And he was always drinking coffee and after every sip he would go: "Ahh!". I mean every two seconds: "Ahh!". And he would take like 40 sips and after everyone: "Ahh!". I had to drop the class.

self._finished_values[message_name] = produce_value(message_class,
message_name,
bytes_for_message,
kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An extremely minor comment: My obsessive-compulsive self tells me that both calls to produce_value() (here and L53) have the arguments not well aligned.

bytes_for_message,
kwargs)
produced_value = produce_value(message_class, message_name, bytes_for_message, kwargs)
del self.processed_objects[message_name] # We don't do this as a pop() in case produce_value raises.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment, can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be marginally more optimal to do self.processed_objects.pop(message_name), but in the (rare, but not unsupported) case that something changes in the receiver class such that produce_value raises at this time (but is caught and subsequently proceeds successfully), we'd already have erased it from processed_objects.

try:
return self._finished_values[message_name]
except KeyError: # suppress might be good here, but it appears to have a performance penalty, and this is a performance-concerned function.
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh - interesting - does .get share a similar performance hit?

Copy link
Contributor Author

@jMyles jMyles Apr 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.get has what appears to be a measurable, but substantially smaller performance hit. Here's what I said in Discord about it:


Interesting: I just ran three test scenarios on the "just in time attribute access", each access an attribute 500000 times from a partially split bytestring.

It's possible that the value has been "finished" (ie, passed to its receiver), so we have to both check that and return the finished value, and then finish the value if it hasn't previously been.

They are both dicts.

I tried with .get(...), with a try block (and except: pass), and suppress.

The times were more different than I thought:

The try block was about 4.2 seconds, the .get was about 4.6 seconds, and the suppress was 6.2 seconds (!).

At some point, we can do some more formal benchmarking. It doesn't feel like the best use of my time right now.

@KPrasch KPrasch added the enhancement New feature or request label Apr 13, 2020
Copy link
Contributor

@vepkenez vepkenez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Why are we doing this now? Where is this needed?

@KPrasch
Copy link
Member

KPrasch commented Apr 13, 2020

@vepkenez - It is needed in nucypher in order to read the domain attribute from a NodeSprout without maturing it, subsequently this will help resolve an outstanding issue regarding unintentional cross-domain node discovery. See nucypher/nucypher#1868

@KPrasch KPrasch merged commit 9a958ca into nucypher:master Apr 13, 2020
@jMyles
Copy link
Contributor Author

jMyles commented Apr 13, 2020

@vepkenez: It's also something like 15% more optimal on its own (though I'm not doing bechmarking right now except with very quick local timeits).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow attribute access to PartiallyKwargifiedBytes

4 participants