Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697723004:
Make regress-crbug-514081 less flaky by having max serialization size (Closed)
DescriptionMake regress-crbug-514081 less flaky by having max serialization size
BUG=v8:5906
R=machenbach@chromium.org
Review-Url: https://codereview.chromium.org/2697723004
Cr-Commit-Position: refs/heads/master@{#43292}
Committed: https://chromium.googlesource.com/v8/v8/+/4dfd5e5ee22693d5c4efc2ef2df369a04f762e15
Patch Set 1 #
Total comments: 8
Patch Set 2 : . #
Total comments: 2
Patch Set 3 : . #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by binji@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.
Description was changed from ========== Make regress-crbug-514081 less flaky by having max serialization size This adds a new d8 flag --max-serializer-memory-usage=<num>. BUG=https://bugs.chromium.org/p/v8/issues/detail?id=5906 R=machenbach@chromium.org ========== to ========== Make regress-crbug-514081 less flaky by having max serialization size This adds a new d8 flag --max-serializer-memory-usage=<num>. BUG=v8:5906 R=machenbach@chromium.org ==========
machenbach@chromium.org changed reviewers: + jochen@chromium.org, yangguo@chromium.org
+yang, +jochen for src change review. jochen also recently fixed a test cases that suffered from large array buffer memory. https://codereview.chromium.org/2697723004/diff/1/src/d8.cc File src/d8.cc (right): https://codereview.chromium.org/2697723004/diff/1/src/d8.cc#newcode2551 src/d8.cc:2551: current_memory_usage_ += size; You could nest the addition within a condition about max_memory_usage_ != 0 as well as it otherwise plays no roll. I assume this might be ok for the purpose of testing and there's no place were current_memory_usage_ is reduced? https://codereview.chromium.org/2697723004/diff/1/src/d8.cc#newcode2632 src/d8.cc:2632: size_t max_memory_usage_; /* For testing when memory exhaustion. */ nit: remove "when"? https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/mjsunit.status File test/mjsunit/mjsunit.status (right): https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/mjsunit.status... test/mjsunit/mjsunit.status:177: 'regress/regress-crbug-514081': [PASS], If you remove the skip, please remove the whole entry here. PASS is the default. https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-crbug-514081.js (right): https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-crbug-514081.js:5: // Flags: --max-serializer-memory-usage=1000 Maybe we need to blacklist this flag on our internal fuzzers to not cause too many bail-outs? Would this be useful to other test cases too where we allocate large array buffers?
On 2017/02/15 09:35:24, Michael Achenbach wrote: > +yang, +jochen for src change review. jochen also recently fixed a test cases > that suffered from large array buffer memory. > > https://codereview.chromium.org/2697723004/diff/1/src/d8.cc > File src/d8.cc (right): > > https://codereview.chromium.org/2697723004/diff/1/src/d8.cc#newcode2551 > src/d8.cc:2551: current_memory_usage_ += size; > You could nest the addition within a condition about max_memory_usage_ != 0 as > well as it otherwise plays no roll. > > I assume this might be ok for the purpose of testing and there's no place were > current_memory_usage_ is reduced? > > https://codereview.chromium.org/2697723004/diff/1/src/d8.cc#newcode2632 > src/d8.cc:2632: size_t max_memory_usage_; /* For testing when memory exhaustion. > */ > nit: remove "when"? > > https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/mjsunit.status > File test/mjsunit/mjsunit.status (right): > > https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/mjsunit.status... > test/mjsunit/mjsunit.status:177: 'regress/regress-crbug-514081': [PASS], > If you remove the skip, please remove the whole entry here. PASS is the default. > > https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/regress/regres... > File test/mjsunit/regress/regress-crbug-514081.js (right): > > https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/regress/regres... > test/mjsunit/regress/regress-crbug-514081.js:5: // Flags: > --max-serializer-memory-usage=1000 > Maybe we need to blacklist this flag on our internal fuzzers to not cause too > many bail-outs? > > Would this be useful to other test cases too where we allocate large array > buffers? Does this have to be a flag, or can we simply hard code this setting in d8?
Removed the flag, and addressed some other comments. https://codereview.chromium.org/2697723004/diff/1/src/d8.cc File src/d8.cc (right): https://codereview.chromium.org/2697723004/diff/1/src/d8.cc#newcode2551 src/d8.cc:2551: current_memory_usage_ += size; On 2017/02/15 09:35:24, Michael Achenbach wrote: > You could nest the addition within a condition about max_memory_usage_ != 0 as > well as it otherwise plays no roll. > > I assume this might be ok for the purpose of testing and there's no place were > current_memory_usage_ is reduced? Right, I added a comment about that. https://codereview.chromium.org/2697723004/diff/1/src/d8.cc#newcode2632 src/d8.cc:2632: size_t max_memory_usage_; /* For testing when memory exhaustion. */ On 2017/02/15 09:35:24, Michael Achenbach wrote: > nit: remove "when"? Done. https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/mjsunit.status File test/mjsunit/mjsunit.status (right): https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/mjsunit.status... test/mjsunit/mjsunit.status:177: 'regress/regress-crbug-514081': [PASS], On 2017/02/15 09:35:24, Michael Achenbach wrote: > If you remove the skip, please remove the whole entry here. PASS is the default. Done. https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-crbug-514081.js (right): https://codereview.chromium.org/2697723004/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-crbug-514081.js:5: // Flags: --max-serializer-memory-usage=1000 On 2017/02/15 09:35:24, Michael Achenbach wrote: > Maybe we need to blacklist this flag on our internal fuzzers to not cause too > many bail-outs? I removed the flag instead, as suggested by Yang. > > Would this be useful to other test cases too where we allocate large array > buffers? No, because this only tests the serialization buffer size.
The CQ bit was checked by binji@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 ========== Make regress-crbug-514081 less flaky by having max serialization size This adds a new d8 flag --max-serializer-memory-usage=<num>. BUG=v8:5906 R=machenbach@chromium.org ========== to ========== Make regress-crbug-514081 less flaky by having max serialization size BUG=v8:5906 R=machenbach@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2697723004/diff/20001/src/d8.cc File src/d8.cc (left): https://codereview.chromium.org/2697723004/diff/20001/src/d8.cc#oldcode2474 src/d8.cc:2474: argv[i] = NULL; Accidental change?
https://codereview.chromium.org/2697723004/diff/20001/src/d8.cc File src/d8.cc (left): https://codereview.chromium.org/2697723004/diff/20001/src/d8.cc#oldcode2474 src/d8.cc:2474: argv[i] = NULL; On 2017/02/16 07:59:32, Yang wrote: > Accidental change? Oops, fixed.
The CQ bit was checked by binji@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.
On 2017/02/17 00:49:13, binji wrote: > https://codereview.chromium.org/2697723004/diff/20001/src/d8.cc > File src/d8.cc (left): > > https://codereview.chromium.org/2697723004/diff/20001/src/d8.cc#oldcode2474 > src/d8.cc:2474: argv[i] = NULL; > On 2017/02/16 07:59:32, Yang wrote: > > Accidental change? > > Oops, fixed. lgtm
The CQ bit was checked by binji@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2697723004/#ps40001 (title: ".")
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": 40001, "attempt_start_ts": 1487357627015160,
"parent_rev": "0f1f54c27bfafeb660ccf0c75021a9ea0cad4e26", "commit_rev":
"4dfd5e5ee22693d5c4efc2ef2df369a04f762e15"}
Message was sent while issue was closed.
Description was changed from ========== Make regress-crbug-514081 less flaky by having max serialization size BUG=v8:5906 R=machenbach@chromium.org ========== to ========== Make regress-crbug-514081 less flaky by having max serialization size BUG=v8:5906 R=machenbach@chromium.org Review-Url: https://codereview.chromium.org/2697723004 Cr-Commit-Position: refs/heads/master@{#43292} Committed: https://chromium.googlesource.com/v8/v8/+/4dfd5e5ee22693d5c4efc2ef2df369a04f7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/4dfd5e5ee22693d5c4efc2ef2df369a04f7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
