New issue
Advanced search Search tips

Issue 850530 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 9
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug-Regression



Sign in to add a comment

Regressions caused by "[async] Expose async hooks to d8"

Project Member Reported by mslekova@chromium.org, Jun 7 2018

Issue description

All the Promises benchmarks degrade at this point: https://chromeperf.appspot.com/report?sid=7217a7f7e93361c48190f612aea11fa1c41ebf5cf6000fb61f36eb6b5f8fd73f&start_rev=52984&end_rev=53575 which probably means it's caused by this commit: https://chromium.googlesource.com/v8/v8/+/3c4d0316e436d62179dfc96dbf270cc809984bd2

Still in theory if we run without the --enable-async-hooks flag, it should have no performance impact. Will check the graphs again now that this is reverted anyway.
 
Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad

commit 8e0f67be3f73c10556f1c93f003c3b8c785cd0ad
Author: Maya Lekova <mslekova@chromium.org>
Date: Thu Jun 21 04:11:17 2018

Reland "[async] Expose async hooks to d8"

This is a reland of 3c4d0316e436d62179dfc96dbf270cc809984bd2

Original change's description:
> [async] Expose async hooks to d8
>
> This implementation follows the Node.js API as a guideline.
>
> Change-Id: I09274ea25ccdbb9794a7440d6c14f26b9febb4f4
> Reviewed-on: https://chromium-review.googlesource.com/1065818
> Commit-Queue: Maya Lekova <mslekova@chromium.org>
> Reviewed-by: Ali Ijaz Sheikh <ofrobots@google.com>
> Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
> Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#53551}

Change-Id: If2114db2ff179c6b07a40bc0c2dac3a41f37aea9
Bug:  chromium:850530 
Reviewed-on: https://chromium-review.googlesource.com/1088890
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53901}
[modify] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/BUILD.gn
[add] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/src/async-hooks-wrapper.cc
[add] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/src/async-hooks-wrapper.h
[modify] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/src/d8.cc
[modify] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/src/d8.h
[modify] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/src/flag-definitions.h
[modify] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/allocation-site-info.js
[add] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/async-hooks/api-methods.js
[add] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/async-hooks/async-await-tree.js
[add] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/async-hooks/chained-promises.js
[add] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/async-hooks/execution-order.js
[add] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/async-hooks/promises-async-await.js
[add] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/es8/async-await-interleaved.js

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/d31e0315147cf98761c45d68bedfdba3cd57102b

commit d31e0315147cf98761c45d68bedfdba3cd57102b
Author: Michael Achenbach <machenbach@chromium.org>
Date: Thu Jun 21 06:28:30 2018

Revert "Reland "[async] Expose async hooks to d8""

This reverts commit 8e0f67be3f73c10556f1c93f003c3b8c785cd0ad.

Reason for revert:
https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Linux%20-%20debug/20949

Original change's description:
> Reland "[async] Expose async hooks to d8"
> 
> This is a reland of 3c4d0316e436d62179dfc96dbf270cc809984bd2
> 
> Original change's description:
> > [async] Expose async hooks to d8
> >
> > This implementation follows the Node.js API as a guideline.
> >
> > Change-Id: I09274ea25ccdbb9794a7440d6c14f26b9febb4f4
> > Reviewed-on: https://chromium-review.googlesource.com/1065818
> > Commit-Queue: Maya Lekova <mslekova@chromium.org>
> > Reviewed-by: Ali Ijaz Sheikh <ofrobots@google.com>
> > Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
> > Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#53551}
> 
> Change-Id: If2114db2ff179c6b07a40bc0c2dac3a41f37aea9
> Bug:  chromium:850530 
> Reviewed-on: https://chromium-review.googlesource.com/1088890
> Commit-Queue: Maya Lekova <mslekova@chromium.org>
> Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#53901}

TBR=ofrobots@google.com,sergiyb@chromium.org,gsathya@chromium.org,bmeurer@chromium.org,mslekova@chromium.org

Change-Id: Id55809a46bc5118103391fdbdfb52415182d3fd4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:850530 
Reviewed-on: https://chromium-review.googlesource.com/1109038
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53902}
[modify] https://crrev.com/d31e0315147cf98761c45d68bedfdba3cd57102b/BUILD.gn
[delete] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/src/async-hooks-wrapper.cc
[delete] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/src/async-hooks-wrapper.h
[modify] https://crrev.com/d31e0315147cf98761c45d68bedfdba3cd57102b/src/d8.cc
[modify] https://crrev.com/d31e0315147cf98761c45d68bedfdba3cd57102b/src/d8.h
[modify] https://crrev.com/d31e0315147cf98761c45d68bedfdba3cd57102b/src/flag-definitions.h
[modify] https://crrev.com/d31e0315147cf98761c45d68bedfdba3cd57102b/test/mjsunit/allocation-site-info.js
[delete] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/async-hooks/api-methods.js
[delete] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/async-hooks/async-await-tree.js
[delete] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/async-hooks/chained-promises.js
[delete] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/async-hooks/execution-order.js
[delete] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/async-hooks/promises-async-await.js
[delete] https://crrev.com/8e0f67be3f73c10556f1c93f003c3b8c785cd0ad/test/mjsunit/es8/async-await-interleaved.js

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 4

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/ade7f55b3a61c6a8191f71248b3b66365104e215

commit ade7f55b3a61c6a8191f71248b3b66365104e215
Author: Maya Lekova <mslekova@chromium.org>
Date: Wed Jul 04 08:28:57 2018

Reland ^2 "[async] Expose async hooks to d8"

This is a reland of 8e0f67be3f73c10556f1c93f003c3b8c785cd0ad

Previously landed as: 3c4d0316e43 / 1065818
Previously landed as: 8e0f67be3f7 / 1088890

Original change's description:
> [async] Expose async hooks to d8
>
> This implementation follows the Node.js API as a guideline.

Bug:  chromium:850530 
Change-Id: I8ba22b11c80328108b197d687826ce0198420c9c
Reviewed-on: https://chromium-review.googlesource.com/1125679
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54191}
[modify] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/BUILD.gn
[add] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/src/async-hooks-wrapper.cc
[add] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/src/async-hooks-wrapper.h
[modify] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/src/d8.cc
[modify] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/src/d8.h
[modify] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/src/flag-definitions.h
[modify] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/test/mjsunit/allocation-site-info.js
[add] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/test/mjsunit/async-hooks/api-methods.js
[add] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/test/mjsunit/async-hooks/async-await-tree.js
[add] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/test/mjsunit/async-hooks/chained-promises.js
[add] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/test/mjsunit/async-hooks/execution-order.js
[add] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/test/mjsunit/async-hooks/promises-async-await.js
[add] https://crrev.com/ade7f55b3a61c6a8191f71248b3b66365104e215/test/mjsunit/es8/async-await-interleaved.js

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 4

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/86fb0feb353930edb55475d8a1d0949363023d46

commit 86fb0feb353930edb55475d8a1d0949363023d46
Author: Yang Guo <yangguo@chromium.org>
Date: Wed Jul 04 09:07:50 2018

Revert "Reland ^2 "[async] Expose async hooks to d8""

This reverts commit ade7f55b3a61c6a8191f71248b3b66365104e215.

Reason for revert: https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Linux/25706

Original change's description:
> Reland ^2 "[async] Expose async hooks to d8"
> 
> This is a reland of 8e0f67be3f73c10556f1c93f003c3b8c785cd0ad
> 
> Previously landed as: 3c4d0316e43 / 1065818
> Previously landed as: 8e0f67be3f7 / 1088890
> 
> Original change's description:
> > [async] Expose async hooks to d8
> >
> > This implementation follows the Node.js API as a guideline.
> 
> Bug:  chromium:850530 
> Change-Id: I8ba22b11c80328108b197d687826ce0198420c9c
> Reviewed-on: https://chromium-review.googlesource.com/1125679
> Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
> Commit-Queue: Maya Lekova <mslekova@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#54191}

TBR=ofrobots@google.com,sergiyb@chromium.org,gsathya@chromium.org,bmeurer@chromium.org,mslekova@chromium.org

Change-Id: Id751136aee175bb3ba75edc780d62cfc9d60ed24
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:850530 
Reviewed-on: https://chromium-review.googlesource.com/1125682
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54195}
[modify] https://crrev.com/86fb0feb353930edb55475d8a1d0949363023d46/BUILD.gn
[delete] https://crrev.com/f3054adce3aed4cd0d4a6932f35cc3e97f1546e5/src/async-hooks-wrapper.cc
[delete] https://crrev.com/f3054adce3aed4cd0d4a6932f35cc3e97f1546e5/src/async-hooks-wrapper.h
[modify] https://crrev.com/86fb0feb353930edb55475d8a1d0949363023d46/src/d8.cc
[modify] https://crrev.com/86fb0feb353930edb55475d8a1d0949363023d46/src/d8.h
[modify] https://crrev.com/86fb0feb353930edb55475d8a1d0949363023d46/src/flag-definitions.h
[modify] https://crrev.com/86fb0feb353930edb55475d8a1d0949363023d46/test/mjsunit/allocation-site-info.js
[delete] https://crrev.com/f3054adce3aed4cd0d4a6932f35cc3e97f1546e5/test/mjsunit/async-hooks/api-methods.js
[delete] https://crrev.com/f3054adce3aed4cd0d4a6932f35cc3e97f1546e5/test/mjsunit/async-hooks/async-await-tree.js
[delete] https://crrev.com/f3054adce3aed4cd0d4a6932f35cc3e97f1546e5/test/mjsunit/async-hooks/chained-promises.js
[delete] https://crrev.com/f3054adce3aed4cd0d4a6932f35cc3e97f1546e5/test/mjsunit/async-hooks/execution-order.js
[delete] https://crrev.com/f3054adce3aed4cd0d4a6932f35cc3e97f1546e5/test/mjsunit/async-hooks/promises-async-await.js
[delete] https://crrev.com/f3054adce3aed4cd0d4a6932f35cc3e97f1546e5/test/mjsunit/es8/async-await-interleaved.js

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 4

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393

commit 916e35d72f4cdc6aaf39174e4f4a1a6b4297e393
Author: Maya Lekova <mslekova@chromium.org>
Date: Wed Jul 04 15:47:16 2018

Reland ^3 "[async] Expose async hooks to d8"

This is a reland of ade7f55b3a61c6a8191f71248b3b66365104e215

Previously landed as: ade7f55b3a6 / 1125679
Previously landed as: 3c4d0316e43 / 1065818
Previously landed as: 8e0f67be3f7 / 1088890

Original change's description:
> Reland ^2 "[async] Expose async hooks to d8"
>
> This is a reland of 8e0f67be3f73c10556f1c93f003c3b8c785cd0ad
>

Bug:  chromium:850530 
Change-Id: I536cfb9443d80d62937d9c3dc6a53b52b209d5c7
Reviewed-on: https://chromium-review.googlesource.com/1125683
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54218}
[modify] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/BUILD.gn
[add] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/src/async-hooks-wrapper.cc
[add] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/src/async-hooks-wrapper.h
[modify] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/src/d8.cc
[modify] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/src/d8.h
[modify] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/src/flag-definitions.h
[modify] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/test/mjsunit/allocation-site-info.js
[add] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/test/mjsunit/async-hooks/api-methods.js
[add] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/test/mjsunit/async-hooks/async-await-tree.js
[add] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/test/mjsunit/async-hooks/chained-promises.js
[add] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/test/mjsunit/async-hooks/execution-order.js
[add] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/test/mjsunit/async-hooks/promises-async-await.js
[add] https://crrev.com/916e35d72f4cdc6aaf39174e4f4a1a6b4297e393/test/mjsunit/es8/async-await-interleaved.js

Status: Fixed (was: Started)
The metrics seem stable 4 days after the reland of the original change. I'm considering the issue fixed now.

Sign in to add a comment