Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1835553002:
Control ES2015 'Tail call elimination' language feature via finch flag. (Closed)
|
Created:
4 years, 9 months ago by Igor Sheludko Modified:
4 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionControl ES2015 'Tail call elimination' language feature via finch flag.
BUG=chromium:596437
LOG=N
Committed: https://crrev.com/39edcd6e2c9978335f9731d099e95471a061c716
Cr-Commit-Position: refs/heads/master@{#383930}
Patch Set 1 : #Patch Set 2 : Added call to base::FeatureList::InitializeInstance() #Patch Set 3 : Fixed redness by moving FeatureList initialization in tests up the call stack. #
Total comments: 1
Patch Set 4 : Giving up... Make feature checking less intrusive. #Patch Set 5 : Check feature in ChromeRenderProcessObserver #Messages
Total messages: 46 (18 generated)
Patchset #1 (id:1) has been deleted
ishell@chromium.org changed reviewers: + jochen@chromium.org
PTAL
The CQ bit was checked by ishell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835553002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835553002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ishell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835553002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835553002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
the bots appear to disagree?
On 2016/03/29 11:13:19, jochen - slow wrote: > the bots appear to disagree? Investigating...
The CQ bit was checked by ishell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835553002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835553002/60001
Description was changed from ========== Control ES2015 'Tail call elimination' language feature via finch flag. BUG=chromium:596437 LOG=N ========== to ========== Control ES2015 'Tail call elimination' language feature via finch flag. BUG=chromium:596437 LOG=N ==========
ishell@chromium.org changed reviewers: + asvitkine@chromium.org, phajdan.jr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ishell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835553002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835553002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Alexey, PTAL. IsolateHolder is created early and In PS2 I tried to initialize FeatureList instance right before the query but some tests started failing in FeatureList::SetInstance() (see base/test/test_suite.cc) because the instance is already initialized. I tried to move the SetInstance call up the call chain (see PS3), but it worked only for some tests. For the others TestSuite::Run() seems to be the only place for calling SetInstance(). So finally (see PS4) I decided to query the FeatureList only when there is an instance created. In case of Chrome the instance is already there. WDYT about this approach? https://codereview.chromium.org/1835553002/diff/60001/base/test/test_suite.cc File base/test/test_suite.cc (left): https://codereview.chromium.org/1835553002/diff/60001/base/test/test_suite.cc... base/test/test_suite.cc:229: base::FeatureList::SetInstance(make_scoped_ptr(new base::FeatureList)); At this moment gin::IsolateHolder (and therefore FeatureList) is already initialized and this call to SetInstance hits assertion failure.
What tests fail if you don't check the existence of FeatureList instance? I think the right solution is to get that addressed and I can take a look at doing this if you point me at the tests.
On 2016/03/29 21:22:45, Alexei Svitkine wrote: > What tests fail if you don't check the existence of FeatureList instance? > > I think the right solution is to get that addressed and I can take a look at > doing this if you point me at the tests. If I just remove the instance check then it will be a PS1. See red and violet bots there.
One of the failures was: [4056:1040:0326/143144:29857234:FATAL:feature_list.cc(175)] Check failed: CheckFeatureIdentity(feature). V8_ES2015_TailCalls From gin_unittests (https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...) This is because you're defining your feature on the stack, which is not correct. A feature must be defined in a static area - so that it's address does not change. You can fix this specific issue by defining it in the anon namespace at the top of the file, for example. (I don't know why your latest patchset doesn't run into this issue.) I will try to look at the other and see if there's other problems I find.
On 2016/03/29 21:38:50, Alexei Svitkine wrote: > One of the failures was: > > [4056:1040:0326/143144:29857234:FATAL:feature_list.cc(175)] Check failed: > CheckFeatureIdentity(feature). V8_ES2015_TailCalls > > From gin_unittests > (https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...) > > This is because you're defining your feature on the stack, which is not correct. > A feature must be defined in a static area - so that it's address does not > change. > > You can fix this specific issue by defining it in the anon namespace at the top > of the file, for example. (I don't know why your latest patchset doesn't run > into this issue.) > > I will try to look at the other and see if there's other problems I find. Not sure about this particular test but many others crashed inside IsEnabled() here: https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list....
I plan to spend time debugging this tomorrow for the other failures and let you know if I find a better solution.
> Not sure about this particular test but many others crashed inside IsEnabled() > here: > https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list.... There were also some number of tests that create HolderInstance but are not not based on TestSuite class (https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sui...)
On 2016/03/29 21:48:15, Alexei Svitkine wrote: > I plan to spend time debugging this tomorrow for the other failures and let you > know if I find a better solution. Thanks! In the mean time I'll try to figure out a better place for IsEnabled() call.
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by ishell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835553002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835553002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
this won't affect the network stack nor PDFium, both of which also execute V8. Is that good enough for you?
On 2016/03/30 07:41:06, jochen - slow wrote: > this won't affect the network stack nor PDFium, both of which also execute V8. > > Is that good enough for you? I think we can start this way or we will completely miss M51. And then we can try to fix all the test issues and move the code back to IsolateHolder.
lgtm
The CQ bit was checked by ishell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835553002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835553002/120001
Message was sent while issue was closed.
Description was changed from ========== Control ES2015 'Tail call elimination' language feature via finch flag. BUG=chromium:596437 LOG=N ========== to ========== Control ES2015 'Tail call elimination' language feature via finch flag. BUG=chromium:596437 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Control ES2015 'Tail call elimination' language feature via finch flag. BUG=chromium:596437 LOG=N ========== to ========== Control ES2015 'Tail call elimination' language feature via finch flag. BUG=chromium:596437 LOG=N Committed: https://crrev.com/39edcd6e2c9978335f9731d099e95471a061c716 Cr-Commit-Position: refs/heads/master@{#383930} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/39edcd6e2c9978335f9731d099e95471a061c716 Cr-Commit-Position: refs/heads/master@{#383930}
Message was sent while issue was closed.
I've filed http://crbug.com/599165 to track fixing using Feature API from gin/ |
