Check nbytes and types before reading data
#3628
Conversation
To avoid reading `data` when it is not needed, try checking `nbytes` and `types` beforehand. If the metadata is already there, continue on without reading `data`. Otherwise fallback to the reading `data`, but do make sure to cache the results of that read to avoid doing it again.
| value = self.data[key] | ||
| except KeyError: | ||
| value = self.actors[key] | ||
| nbytes = self.nbytes[key] or sizeof(value) |
There was a problem hiding this comment.
This appeared to just use getitem on nbytes, which AFAICT is a dict. So maybe getting value is never needed? If it is, maybe we should be changing this to .get(...).
There was a problem hiding this comment.
Yeah, good catch. I don't know.
There was a problem hiding this comment.
I went ahead and used .get(...) instead and raised issue ( #3629 ) to determine whether the fallback to get value is needed.
If it is needed, this should fix a bug. If not, we can go back and remove it separately 🙂
|
+1 from me. Thanks @jakirkham |
|
Thanks for the guidance Matt 😄 |
|
Not sure I did anything, but I'll take all the credit I can get :)
…On Mon, Mar 23, 2020 at 5:05 PM jakirkham ***@***.***> wrote:
Thanks for the guidance Matt 😄
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3628 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKZTAW45IG3AFEXIAHZHDRI72ONANCNFSM4LSH7FRQ>
.
|
Appears GitHub CI failed to checkout the code and clicking restart in the UI does not work. So pushing a dummy commit to restart it.
pentschev
left a comment
pentschev
left a comment
There was a problem hiding this comment.
This was a very sneaky piece of code, nice catch @jakirkham !
|
Thanks everyone for the reviews and @jakirkham for the work. Merging in |
5 participants
To avoid reading
datawhen it is not needed, try checkingnbytesandtypesbeforehand. If the metadata is already there, continue on without readingdata. Otherwise fallback to the readingdata, but do make sure to cache the results of that read to avoid doing it again.cc @madsbk @mrocklin