New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 19
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-12-19
OS: Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment
link

Issue 907032: Regression: Delay is seen in zipping a file for the first time

Reported by kebalaji@chromium.org, Nov 20 Project Member

Issue description

Chrome Version: 72.0.3612.0/11278.0.0 dev-channel Kip,Daisy,Celes
OS: Chrome OS

What steps will reproduce the problem?
(1)Sign-in to user>> Open Files App and take a screenshot
(2)Zip the screenshot and observe delay is seen in zipping a file for the first time 

Actual: Delay is seen in zipping a file for the first time 
Expected: No such delay should be seen 

This is a Regression issue as same is working fine in M71 beta

Attaching screencasts for reference..

NOTE: Issue is not seen from second time
 
ActualZipping.mp4
13.1 MB View Download
ExpectedZipping.mp4
4.5 MB View Download

Comment 1 by amistry@chromium.org, Nov 23

Cc: amistry@chromium.org
Labels: CrOSFilesFeature-Zip
Status: Available (was: Untriaged)

Comment 2 by slangley@chromium.org, Dec 10

Cc: -amistry@chromium.org
Owner: amistry@chromium.org
Status: Assigned (was: Available)

Comment 3 by amistry@chromium.org, Dec 14

What's happening is that the first time a user runs some zip functionality, the zip archiver NaCl module is loaded which is translated from PNaCl. The PNaCl translation is what's taking a long time. The archiver used to load the NaCl module always on login, which is inefficient, but it had the side effect of causing a PNaCl translation the first time a user logs in. Note, the translation will only happen the first time because Chrome OS caches the translated NaCl binary.

Going back to always loading the NaCl module on every login is an unnecessary inefficiency (and causes significant slowdowns in browser tests). However, doing it purely lazily causes this regression on first use. A compromise is to do it the first time a user logs in or when Chrome OS is updated. chrome.runtime.onInstalled gives us this capability.

Comment 4 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86fa5708fc2dce1bf5f418bb640fc305b56c491e

commit 86fa5708fc2dce1bf5f418bb640fc305b56c491e
Author: Anand K. Mistry <amistry@chromium.org>
Date: Fri Dec 14 05:50:07 2018

Preload the zip archiver NaCl module on first login or after update.

Loading the module the first time causes a PNaCl translation, which is
cached for subsequent loads. Loading lazily causes a performance
regression the first time the user uses zip functionaility.

BUG= 907032 

Change-Id: Iba4ee63ec0f940a1bfaa4bbbf52801a5a5dbea5a
Reviewed-on: https://chromium-review.googlesource.com/c/1377953
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616590}
[modify] https://crrev.com/86fa5708fc2dce1bf5f418bb640fc305b56c491e/chrome/browser/resources/chromeos/zip_archiver/js/background.js

Comment 5 by noel@chromium.org, Dec 14

#3 Agree, no easy compromise here.  Putting things back to the way they were, with some improvement via patch #4, SGTM.  A reasonable compromise, given the imperfect situation, I believe.   Thank you amistry@ for taking on this bug.  Not easy.

Comment 6 by amistry@chromium.org, Dec 17

NextAction: 2018-12-19
Setting NextAction to wait for canary before requesting 72 merge.

Comment 7 by monor...@bugs.chromium.org, Dec 19

The NextAction date has arrived: 2018-12-19

Comment 8 by amistry@chromium.org, Dec 19

Labels: Merge-Request-72
Status: Fixed (was: Assigned)
Verified on canary 73.0.3644.0.

Comment 9 by sheriffbot@chromium.org, Dec 19

Project Member
Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 10 by dgagnon@google.com, Dec 20

Labels: -Hotlist-Merge-Review -Merge-Review-72 Merge-Approved-72

Comment 11 by bugdroid1@chromium.org, Dec 20

Project Member
Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bdbffae85372c96c3fa311c13f7281307d505094

commit bdbffae85372c96c3fa311c13f7281307d505094
Author: Anand K. Mistry <amistry@chromium.org>
Date: Thu Dec 20 23:23:47 2018

Preload the zip archiver NaCl module on first login or after update.

Loading the module the first time causes a PNaCl translation, which is
cached for subsequent loads. Loading lazily causes a performance
regression the first time the user uses zip functionaility.

BUG= 907032 

Change-Id: Iba4ee63ec0f940a1bfaa4bbbf52801a5a5dbea5a
Reviewed-on: https://chromium-review.googlesource.com/c/1377953
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#616590}(cherry picked from commit 86fa5708fc2dce1bf5f418bb640fc305b56c491e)
Reviewed-on: https://chromium-review.googlesource.com/c/1388044
Reviewed-by: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#492}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/bdbffae85372c96c3fa311c13f7281307d505094/chrome/browser/resources/chromeos/zip_archiver/js/background.js

Comment 12 by cr-audit...@appspot.gserviceaccount.com, Dec 20

Project Member
Labels: CommitLog-Audit-Violation Merge-Without-Approval
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision bdbffae85372c96c3fa311c13f7281307d505094 was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
 - AcknowledgeMerge: Notification Required --

Comment 13 by cr-audit...@appspot.gserviceaccount.com, Dec 20

Project Member
Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/bdbffae85372c96c3fa311c13f7281307d505094

Commit: bdbffae85372c96c3fa311c13f7281307d505094
Author: amistry@chromium.org
Commiter: amistry@chromium.org
Date: 2018-12-20 23:23:47 +0000 UTC

Preload the zip archiver NaCl module on first login or after update.

Loading the module the first time causes a PNaCl translation, which is
cached for subsequent loads. Loading lazily causes a performance
regression the first time the user uses zip functionaility.

BUG= 907032 

Change-Id: Iba4ee63ec0f940a1bfaa4bbbf52801a5a5dbea5a
Reviewed-on: https://chromium-review.googlesource.com/c/1377953
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#616590}(cherry picked from commit 86fa5708fc2dce1bf5f418bb640fc305b56c491e)
Reviewed-on: https://chromium-review.googlesource.com/c/1388044
Reviewed-by: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#492}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Sign in to add a comment