New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 678583 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Last visit > 30 days ago
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

1.9%-2.9% regression in v8.browsing_mobile_ignition at 440373:440504

Project Member Reported by rmcilroy@chromium.org, Jan 5 2017

Issue description

See the link to graphs below.
 
Cc: eholk@chromium.org
Owner: eholk@chromium.org

=== PERF REGRESSION ===


=== Auto-CCing suspected CL author eholk@chromium.org ===

Hi eholk@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : [wasm] sundry trap handler fixes
Author  : eholk
Commit description:
  
This CL includes several small bug fixes for trap handlers. Among the changes:

* Use the correct representation for ProtectedLoads, enabling protected loads of
  floating point types.

* Including the protected instruction list in what gets serialized for Code
  objects. This is needed to allow deserialization for Wasm modules to work.

* Get the context needed to through and exception from the Isolate rather than
  getting it as a parameter to the Protected instructions. Passing it in as an
  argument is problematic when code is compiled ahead of time, as the context
  may not be known yet. The new approach is similar to how it works for TrapIf
  and TrapUnless.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5277

Review-Url: https://codereview.chromium.org/2591903002
Cr-Commit-Position: refs/heads/master@{#41907}
Commit  : 5fa423d7c28cfb058293cd2b1c21e6fd4df4813f
Date    : Thu Dec 22 00:31:59 2016


===== TESTED REVISIONS =====
Revision                       Mean     Std Dev  N  Good?
chromium@440408                2718824  31060.7  6  good
chromium@440415                2718913  37689.9  6  good
chromium@440416                2714057  54143.9  6  good
chromium@440416,v8@c8ce0cf99e  2734158  10511.9  6  good
chromium@440416,v8@c46f98bced  2719196  34348.0  6  good
chromium@440416,v8@5fa423d7c2  2776329  7270.79  6  bad    <--
chromium@440416,v8@86e2a19991  2772712  36410.1  6  bad
chromium@440417                2761240  27543.7  6  bad
chromium@440418                2763877  42640.5  9  bad
chromium@440420                2765399  5398.08  6  bad
chromium@440431                2762741  29608.3  6  bad

Bisect job ran on: android_nexus9_perf_bisect
Bug ID: 678583

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=browse.news.washingtonpost v8.browsing_mobile_ignition
Test Metric: memory:chrome:renderer_processes:reported_by_chrome:code_and_metadata_size_avg/browse_news/browse_news_washingtonpost
Relative Change: 1.62%

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus9_perf_bisect/builds/2348
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8991305815923103952


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5907021528825856

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!
Status: Assigned (was: Untriaged)
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 31 2017

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

commit 91f8a063cc508f51a18ad9c69faf032552c9d012
Author: eholk <eholk@chromium.org>
Date: Tue Jan 31 02:25:57 2017

[wasm] Move protected instruction info to RelocInfo

Previously this information was encoded in a FixedArray dangling off the
Code object. This extra field seems to be responsible for increased memory
usage, as seen in the linked bugs. In this change, we instead encode this
in the RelocInfo and remove the field from the Code object.

BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=678583
BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=671180
BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=670733

Review-Url: https://codereview.chromium.org/2651833003
Cr-Commit-Position: refs/heads/master@{#42802}

[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/assembler.cc
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/assembler.h
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/compiler/code-generator.cc
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/compiler/code-generator.h
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/compiler/pipeline.cc
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/compiler/wasm-compiler.cc
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/compiler/wasm-compiler.h
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/compiler/x64/code-generator-x64.cc
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/factory.cc
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/objects-body-descriptors-inl.h
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/objects-inl.h
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/objects.h
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/trap-handler/trap-handler.h
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/wasm/wasm-module.cc
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/x64/assembler-x64.cc
[modify] https://crrev.com/91f8a063cc508f51a18ad9c69faf032552c9d012/src/x64/assembler-x64.h

Comment 6 by benhenry@google.com, Jul 14 2017

Status: WontFix (was: Assigned)
This regression is too old to be actionable.

Sign in to add a comment