Buffer in Pickler to improve performance. #27720
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D17847174 |
jjlilley
force-pushed
the
export-D17847174
branch
from
496b083 to
150e5c7
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D17847174 |
Summary: Pull Request resolved: pytorch#27720 This change adds a small fixed-size buffer to Pickler to avoid calling writer_() and the associated downstream checks on a per-opcode/per-byte basis. We end up still doing a bounds check in the common case, but the memcpy() is a fixed size. And we reduce the number of backend calls. In practice, this change speeds up the Pickle1MInts benchmark for me locally from roughly 56msec to 22msec. Additionally, in this change we convert a few pushIValue() on typed lists, where we know the type to be double/int/boot to be pushInt() to bypass a bit of logic. We should additionally change the Unpickler, though keeping this separate, since the std::function<> prototype needs to be changed for this to work (i.e. return size_t rather than bool). Test Plan: buck test mode/dev-nosan caffe2/test:... Benchmark in experimental/jeremyl/c2/SerializationBench.cpp (run in mode/opt) Differential Revision: D17847174 fbshipit-source-id: 30eace0187f71d9fff13a53013227b780f93c6c0
jjlilley
force-pushed
the
export-D17847174
branch
from
150e5c7 to
fd49e74
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D17847174 |
resistor
reviewed
Oct 12, 2019
resistor
approved these changes
Oct 13, 2019
Contributor
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@jjlilley is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Author
|
Would you mind stamping #28043 ? I used the web-based export-from-phabricator originally with this diff, which alas is suffering from an Infra failure, and apparently using the web tool makes ghexport not work. The change 28043 is identical except that I addressed the two pending review comments:
Thanks! |
Contributor
thiagocrepaldi
pushed a commit
to thiagocrepaldi/pytorch
that referenced
this pull request
Feb 4, 2020
Summary: This change adds a small fixed-size buffer to Pickler to avoid calling writer_() and the associated downstream checks on a per-opcode/per-byte basis. We end up still doing a bounds check in the common case, but the memcpy() is a fixed size. And we reduce the number of backend calls. In practice, this change speeds up the Pickle1MInts benchmark for me locally from roughly 56msec to 22msec. Additionally, in this change we convert a few pushIValue() on typed lists, where we know the type to be double/int/boot to be pushInt() to bypass a bit of logic. We should additionally change the Unpickler, though keeping this separate, since the std::function<> prototype needs to be changed for this to work (i.e. return size_t rather than bool). Pull Request resolved: pytorch#27720 Test Plan: buck test mode/dev-nosan caffe2/test:... Benchmark in experimental/jeremyl/c2/SerializationBench.cpp (run in mode/opt) Differential Revision: D17847174 Pulled By: jjlilley fbshipit-source-id: 22e5e5fd33f1a369c124ea5aac7880538e2bf6a0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This change adds a small fixed-size buffer to Pickler to
avoid calling writer_() and the associated downstream checks
on a per-opcode/per-byte basis.
We end up still doing a bounds check in the common case,
but the memcpy() is a fixed size. And we reduce the number
of backend calls.
In practice, this change speeds up the Pickle1MInts benchmark
for me locally from roughly 56msec to 22msec.
Additionally, in this change we convert a few pushIValue() on
typed lists, where we know the type to be double/int/boot to be
pushInt() to bypass a bit of logic.
We should additionally change the Unpickler, though keeping
this separate, since the std::function<> prototype needs to be
changed for this to work (i.e. return size_t rather than bool).