Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2481853002:
[turbofan] Use a vector instead of a set in move optimizer. (Closed)
Description[turbofan] Use a vector instead of a set in move optimizer.
The set of operands are really small, so STL set performs really poorly.
In Octane/TypeScript, I see move optimization going from >300ms to <100ms.
Committed: https://crrev.com/6a08157ef1b6585417dfc056c9b11acc322089f0
Cr-Commit-Position: refs/heads/master@{#40835}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comment #
Messages
Total messages: 23 (13 generated)
The CQ bit was checked by jarin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jarin@chromium.org changed reviewers: + bmeurer@chromium.org, mtrofin@chromium.org
Could you take a look, please?
Do we know what underperforms in the STL set - is it insertion? Is it because it has to rebalance? https://codereview.chromium.org/2481853002/diff/1/src/compiler/move-optimizer.cc File src/compiler/move-optimizer.cc (right): https://codereview.chromium.org/2481853002/diff/1/src/compiler/move-optimizer... src/compiler/move-optimizer.cc:37: set_->push_back(op); Are we sure the previous implementation didn't assume set_ was ensuring unique entries? https://codereview.chromium.org/2481853002/diff/1/src/compiler/move-optimizer.h File src/compiler/move-optimizer.h (right): https://codereview.chromium.org/2481853002/diff/1/src/compiler/move-optimizer... src/compiler/move-optimizer.h:56: ZoneVector<InstructionOperand> operand_buffer1; could you add a comment as to what these guys are, and why 2? I'm guessing because we tend to use 2 operand sets (to/from)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Neat, LGTM from my side.
The CQ bit was checked by jarin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [turbofan] Use a vector with a set in move optimizer. The set of operands are really small, so STL set performs really poorly. In Octane/TypeScript, I see move optimization going from >300ms to <100ms. ========== to ========== [turbofan] Use a vector instead of a set in move optimizer. The set of operands are really small, so STL set performs really poorly. In Octane/TypeScript, I see move optimization going from >300ms to <100ms. ==========
As for why it is slow - yes, it is the insertion. For small sets, you cannot beat the linear search. For large sets, STL set might be better fit, but even there it has poor cache locality, and hash table is often much better choice. Here it looks like unordered vector is just fine. https://codereview.chromium.org/2481853002/diff/1/src/compiler/move-optimizer.cc File src/compiler/move-optimizer.cc (right): https://codereview.chromium.org/2481853002/diff/1/src/compiler/move-optimizer... src/compiler/move-optimizer.cc:37: set_->push_back(op); On 2016/11/07 16:20:09, Mircea Trofin wrote: > Are we sure the previous implementation didn't assume set_ was ensuring unique > entries? I actually tried both with and without ensuring uniqueness, and the version without uniqueness performs much better (the uniqueness check was expensive). https://codereview.chromium.org/2481853002/diff/1/src/compiler/move-optimizer.h File src/compiler/move-optimizer.h (right): https://codereview.chromium.org/2481853002/diff/1/src/compiler/move-optimizer... src/compiler/move-optimizer.h:56: ZoneVector<InstructionOperand> operand_buffer1; On 2016/11/07 16:20:09, Mircea Trofin wrote: > could you add a comment as to what these guys are, and why 2? I'm guessing > because we tend to use 2 operand sets (to/from)? Done.
The CQ bit was unchecked by jarin@chromium.org
The CQ bit was checked by jarin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2481853002/#ps20001 (title: "Comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Use a vector instead of a set in move optimizer. The set of operands are really small, so STL set performs really poorly. In Octane/TypeScript, I see move optimization going from >300ms to <100ms. ========== to ========== [turbofan] Use a vector instead of a set in move optimizer. The set of operands are really small, so STL set performs really poorly. In Octane/TypeScript, I see move optimization going from >300ms to <100ms. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
On 2016/11/08 14:00:05, commit-bot: I haz the power wrote: > Committed patchset #2 (id:20001) Is the uniqueness check necessary, though? It appears no tests fail, but that may be because we don't have coverage.
Message was sent while issue was closed.
On 2016/11/08 15:28:33, Mircea Trofin wrote: > On 2016/11/08 14:00:05, commit-bot: I haz the power wrote: > > Committed patchset #2 (id:20001) > > Is the uniqueness check necessary, though? It appears no tests fail, but that > may be because we don't have coverage. It is a set with find and insert operations, so uniqueness does not matter. Or am I missing something?
Message was sent while issue was closed.
On 2016/11/08 15:31:28, Jarin wrote: > On 2016/11/08 15:28:33, Mircea Trofin wrote: > > On 2016/11/08 14:00:05, commit-bot: I haz the power wrote: > > > Committed patchset #2 (id:20001) > > > > Is the uniqueness check necessary, though? It appears no tests fail, but that > > may be because we don't have coverage. > > It is a set with find and insert operations, so uniqueness does not matter. Or > am I missing something? LGTM - we don't remove, missed that.
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Use a vector instead of a set in move optimizer. The set of operands are really small, so STL set performs really poorly. In Octane/TypeScript, I see move optimization going from >300ms to <100ms. ========== to ========== [turbofan] Use a vector instead of a set in move optimizer. The set of operands are really small, so STL set performs really poorly. In Octane/TypeScript, I see move optimization going from >300ms to <100ms. Committed: https://crrev.com/6a08157ef1b6585417dfc056c9b11acc322089f0 Cr-Commit-Position: refs/heads/master@{#40835} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6a08157ef1b6585417dfc056c9b11acc322089f0 Cr-Commit-Position: refs/heads/master@{#40835} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
