Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2881493004:
Marshal 64 bit byte counts as double to the profiler. (Closed)
|
Created:
3 years, 7 months ago by Sigurður Ásgeirsson Modified:
3 years, 7 months ago CC:
chromium-reviews, danakj+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMarshal 64 bit byte counts as double through to JavaScript.
As-is, byte counts larger than 31 bits are getting truncated and/or
mis-interpreted as negative numbers, which does no good on the summing.
Also explicitly test the transitions from <31 bit to >31 bit to >32 bits
which I originally suspected of causing the negative aggregates.
BUG=644385
TBR=jochen@chromium.org
Review-Url: https://codereview.chromium.org/2881493004
Cr-Commit-Position: refs/heads/master@{#471356}
Committed: https://chromium.googlesource.com/chromium/src/+/03ce267472a3f16830796d5bd04d79371d61afaf
Patch Set 1 #Patch Set 2 : Fix the serialization unit test. #
Total comments: 3
Messages
Total messages: 23 (14 generated)
siggi@chromium.org changed reviewers: + chrisha@chromium.org, gab@chromium.org, jochen@chromium.org
This fixes a problem where the byte sum aggregates go negative in the profiler. Chris for functionality, Gab & Jochen for OWNERs pretty please.
lgtm
The CQ bit was checked by siggi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by siggi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Now actually works - gentle nudge for OWNERS.
lgtm https://codereview.chromium.org/2881493004/diff/20001/base/tracked_objects_un... File base/tracked_objects_unittest.cc (right): https://codereview.chromium.org/2881493004/diff/20001/base/tracked_objects_un... base/tracked_objects_unittest.cc:350: // In the 32 bit implementation, this tests the case where the low-order in the "64 bit implementation"?
Thanks! https://codereview.chromium.org/2881493004/diff/20001/base/tracked_objects_un... File base/tracked_objects_unittest.cc (right): https://codereview.chromium.org/2881493004/diff/20001/base/tracked_objects_un... base/tracked_objects_unittest.cc:350: // In the 32 bit implementation, this tests the case where the low-order On 2017/05/12 15:07:52, gab wrote: > in the "64 bit implementation"? No, I specifically mean the 32 bit implementation, the one with all the wonky atomics crap. I suspected the negative values were emerging from the way I concatenate two 32 bit (signed) AtomicIntegers to a 64 bit signed integer. This is essentially a regression test for screwing that up in the future. I can't think of any way to screw up the 64 bit case :).
https://codereview.chromium.org/2881493004/diff/20001/base/tracked_objects_un... File base/tracked_objects_unittest.cc (right): https://codereview.chromium.org/2881493004/diff/20001/base/tracked_objects_un... base/tracked_objects_unittest.cc:350: // In the 32 bit implementation, this tests the case where the low-order On 2017/05/12 15:11:21, Sigurður Ásgeirsson wrote: > On 2017/05/12 15:07:52, gab wrote: > > in the "64 bit implementation"? > > No, I specifically mean the 32 bit implementation, the one with all the wonky > atomics crap. I suspected the negative values were emerging from the way I > concatenate two 32 bit (signed) AtomicIntegers to a 64 bit signed integer. This > is essentially a regression test for screwing that up in the future. > I can't think of any way to screw up the 64 bit case :). Ah, indeed, right :)
Description was changed from ========== Marshal 64 bit byte counts as double through to JavaScript. As-is, byte counts larger than 31 bits are getting truncated and/or mis-interpreted as negative numbers, which does no good on the summing. Also explicitly test the transitions from <31 bit to >31 bit to >32 bits which I originally suspected of causing the negative aggregates. BUG=644385 ========== to ========== Marshal 64 bit byte counts as double through to JavaScript. As-is, byte counts larger than 31 bits are getting truncated and/or mis-interpreted as negative numbers, which does no good on the summing. Also explicitly test the transitions from <31 bit to >31 bit to >32 bits which I originally suspected of causing the negative aggregates. BUG=644385 TBR=jochen@chromium.org ==========
On 2017/05/12 15:13:41, gab wrote: > https://codereview.chromium.org/2881493004/diff/20001/base/tracked_objects_un... > File base/tracked_objects_unittest.cc (right): > > https://codereview.chromium.org/2881493004/diff/20001/base/tracked_objects_un... > base/tracked_objects_unittest.cc:350: // In the 32 bit implementation, this > tests the case where the low-order > On 2017/05/12 15:11:21, Sigurður Ásgeirsson wrote: > > On 2017/05/12 15:07:52, gab wrote: > > > in the "64 bit implementation"? > > > > No, I specifically mean the 32 bit implementation, the one with all the wonky > > atomics crap. I suspected the negative values were emerging from the way I > > concatenate two 32 bit (signed) AtomicIntegers to a 64 bit signed integer. > This > > is essentially a regression test for screwing that up in the future. > > I can't think of any way to screw up the 64 bit case :). > > Ah, indeed, right :) TBRing to jochen, cause I think he'll be drinking beer by now, and I want this in before the weekend.
The CQ bit was checked by siggi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2881493004/#ps20001 (title: "Fix the serialization unit test.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494609194199460,
"parent_rev": "01f2451fdb2d51d41106da071716e9d8349e66b4", "commit_rev":
"03ce267472a3f16830796d5bd04d79371d61afaf"}
Message was sent while issue was closed.
Description was changed from ========== Marshal 64 bit byte counts as double through to JavaScript. As-is, byte counts larger than 31 bits are getting truncated and/or mis-interpreted as negative numbers, which does no good on the summing. Also explicitly test the transitions from <31 bit to >31 bit to >32 bits which I originally suspected of causing the negative aggregates. BUG=644385 TBR=jochen@chromium.org ========== to ========== Marshal 64 bit byte counts as double through to JavaScript. As-is, byte counts larger than 31 bits are getting truncated and/or mis-interpreted as negative numbers, which does no good on the summing. Also explicitly test the transitions from <31 bit to >31 bit to >32 bits which I originally suspected of causing the negative aggregates. BUG=644385 TBR=jochen@chromium.org Review-Url: https://codereview.chromium.org/2881493004 Cr-Commit-Position: refs/heads/master@{#471356} Committed: https://chromium.googlesource.com/chromium/src/+/03ce267472a3f16830796d5bd04d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/03ce267472a3f16830796d5bd04d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
