New issue
Advanced search Search tips

Issue 878302 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 28
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-08-30
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Webpage with WebAssembly starts up slowly after tab refresh

Project Member Reported by ahaas@chromium.org, Aug 28

Issue description

1. Start up Version 70.0.3530.0 (Official Build) canary (64-bit) on Windows 10
2. Launch https://fabric.web-platform.io/ 
3. Observe that Acad Editor boots up
4. Wait for a few seconds
5. Press F5
6. Observe that the second load takes a very long time, 30 seconds instead of 5 seconds on the original load.
 
Status: Fixed (was: Assigned)
This issue got fixed in https://crrev.com/c/1190302

[wasm] Stop wasm compilation before js compilation in Isolate::Deinit

{Isolate::Deinit} waits for all created
{OptimizingCompiler::CompileTask}s to finish. However, these
CompileTasks run in the background and can be blocked by other tasks
which run in the background, e.g. WebAssembly compilation tasks. With
this CL we stop WebAssembly compilation tasks before we wait for
the {optimizingCompiler:::CompileTask}s.

R=mstarzinger@chromium.org
CC=jarin@chromium.org

Change-Id: I1549c1babdebc2e951aef5e48d0aa8130884fb7d
Reviewed-on: https://chromium-review.googlesource.com/1190302
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55429}
Cc: hablich@chromium.org
Labels: Merge-Request-69
I request a merge for this issue once we have some canary coverage. This is a user-visible issue, especially in benchmarks when the same page may be measured repeatedly. In addition the change is small and not risky.
Project Member

Comment 3 by sheriffbot@chromium.org, Aug 28

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
The problem here is that WebAssembly compilation did not get aborted properly by a tab refresh. This can cause OOMs after the refresh, see http://crash/d617df9bec16e22d.
hablich@ for M69 merge review.
NextAction: 2018-08-29
Pls update bug with canary result tomorrow. Thank you.
Note: We're planning to cut M69 stable RC today and the change is not yet made to canary.
Labels: -Merge-Review-69 Merge-Approved-69
Merge is fine after canary coverage
The NextAction date has arrived: 2018-08-29
NextAction: 2018-08-30
It seems like there has not been a canary yet which contains this change.
ahaas@, pls merge the change listed at #1 to canary branch 3535 so we can trigger new canary from same branch. Thank you.
I just cherry-picked it. Please note that I didn't assign any new version. If the Chromium build files are referencing the head of https://chromium.googlesource.com/v8/v8/+log/chromium/3535, the change should be picked up.
Thank you hablich@.

Triggered new canary 70.0.3535.5 with merge listed at #12. Pls verify this change on 70.0.3535.5 or higher.
The NextAction date has arrived: 2018-08-30
Labels: -Merge-Approved-69 Merge-Request-69
Hi! I just saw that the patch does not apply cleanly on V8 6.9. There is a similar change in the code of M69 which would fix the problem. However, we don't have any canary coverage for that change. Also we are not 100% convinced that that change does not have side effects, although we don't expect any. hablich@, do you think we should still merge the change to M69? Just to repeat, the effect of this issue is an increased startup-time (30 seconds instead of 8 seconds) when the user refreshes the tab, and maybe OOMs after tab refresh.
Project Member

Comment 16 by sheriffbot@chromium.org, Aug 30

Labels: -Merge-Request-69 Merge-Review-69
This bug requires manual review: We are only 4 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Per comment #15 and chat with ahaas@, this change won't be included in M69 stable promotion next week.
Labels: -Hotlist-Merge-Review -Merge-Review-69
Removing merge label per #17
Labels: Merge-Review-69
Pls leave "Merge-Review-69" label as it is as we may have to consider this merge for M69 respin (if any) in future.
Any update on this for future M69 respin (if any)? Can't this simply wait until M70?
M69 got promoted to stable today.
This is P2, not "RBS" and per comment #15, the patch does not apply cleanly on V8 6.9. 

So i think it is good to wait until M70. Pls let me know if there is any other concern here. Thank you.
Labels: -Type-Bug -Pri-2 -Merge-Review-69 Merge-Approved-69 Pri-1 Type-Bug-Regression
Please merge to 6.9. I corrected priority and bug type as this is a regression. Let's get this onto the next respin.
Project Member

Comment 23 by bugdroid1@chromium.org, Sep 5

Labels: merge-merged-6.9
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e56148b0b55b892afc12d1e069c62969ca7b7ce2

commit e56148b0b55b892afc12d1e069c62969ca7b7ce2
Author: Andreas Haas <ahaas@chromium.org>
Date: Wed Sep 05 17:13:00 2018

Merged: [wasm] Stop wasm compilation before js compilation in Isolate::Deinit

{Isolate::Deinit} waits for all created
{OptimizingCompiler::CompileTask}s to finish. However, these
CompileTasks run in the background and can be blocked by other tasks
which run in the background, e.g. WebAssembly compilation tasks. With
this CL we stop WebAssembly compilation tasks before we wait for
the {optimizingCompiler:::CompileTask}s.

Revision: a5f627d8eed2019abc802130a30d2a5a3323fc22

NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true

Bug:  chromium:878302 

R=mstarzinger@chromium.org

Change-Id: I9c1148f8037794144dd561ba103f19f096bad49d
Reviewed-on: https://chromium-review.googlesource.com/1207490
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.9@{#41}
Cr-Branched-From: d7b61abe7b48928aed739f02bf7695732d359e7e-refs/heads/6.9.427@{#1}
Cr-Branched-From: b7e108d6016bf6b7de3a34e6d61cb522f5193460-refs/heads/master@{#54504}
[modify] https://crrev.com/e56148b0b55b892afc12d1e069c62969ca7b7ce2/src/isolate.cc

Labels: -Merge-Approved-69
This is already merged to M69 at #23.

Sign in to add a comment