New issue
Advanced search Search tips

Issue 913553 link

Starred by 5 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-12-17
OS: ----
Pri: 2
Type: Bug-Regression

Blocked on:
issue 913491



Sign in to add a comment

Regressions from CallApiCallback refactoring

Project Member Reported by npm@chromium.org, Dec 10

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=913553

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=03dac30d29281cf2defe38bb12a0e8a8e87d3aebbd6c7fb60e20c1add7d66d79


Bot(s) for this bug's original alert(s):

mac-10_13_laptop_high_end-perf

blink_perf.dom - Benchmark documentation link:
  https://bit.ly/blink-perf-benchmarks
Cc: xidac...@chromium.org odejesush@chromium.org pbos@chromium.org jgruber@chromium.org
Owner: odejesush@chromium.org
Status: Assigned (was: Untriaged)
📍 Found significant differences after each of 4 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/12d69832140000

[nojit] Refactor CallApiCallback calling convention by jgruber@chromium.org
https://chromium.googlesource.com/v8/v8/+/c6b0e12e4e664d82cdbbfddca273546b82f98f5d
modify-element-id: 418.5 → 466.4 (+47.85)

[Code health] Do clampTo instead of static_cast by xidachen@chromium.org
https://chromium.googlesource.com/chromium/src/+/242556d729e87df3bf70c0189ecb3dc51ced73c9
modify-element-id: 463.8 → 472.2 (+8.396)

Reset tab-icon animations for loading -> waiting by pbos@chromium.org
https://chromium.googlesource.com/chromium/src/+/5964e2682f6d392d82752c0760deb30633059a47
modify-element-id: 463.3 → 453.1 (-10.27)

Add new chooser methods to SiteSettingsHandler by odejesush@chromium.org
https://chromium.googlesource.com/chromium/src/+/53cf78ca5207a49b798e94c81fffca5085525c63
modify-element-id: 464.2 → 475.6 (+11.39)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Benchmark documentation link:
  https://bit.ly/blink-perf-benchmarks
NextAction: 2018-12-13
Owner: jgruber@chromium.org
Summary: Regressions from CallApiCallback refactoring (was: 15% regression in blink_perf.dom at 613510:613789)
Pinpoint clearly points to 

[nojit] Refactor CallApiCallback calling convention by jgruber@chromium.org
https://chromium.googlesource.com/v8/v8/+/c6b0e12e4e664d82cdbbfddca273546b82f98f5d
modify-element-id: 418.5 → 466.4 (+47.85)

That CL may indeed affect extreme dom-heavy workloads, although I'm surprised it's so much. I'll take a look.
Cc: toyoshim@chromium.org
 Issue 912011  has been merged into this issue.
Blockedon: 913491
 Issue 913552  has been merged into this issue.
Issue 911998 has been merged into this issue.
For some reason the linked graphs from merged bugs weren't merged correctly into this bug? At least I don't see them at https://chromeperf.appspot.com/group_report?bug_id=913553 :{
Reassigned merged alerts manually.
The NextAction date has arrived: 2018-12-13
Cc: yangguo@chromium.org jarin@chromium.org bmeu...@chromium.org
NextAction: 2018-12-17
Had a closer look at dromaeo / dom / http___dromaeo.com?dom-query since that shows a clean regression.

https://chromeperf.appspot.com/report?sid=ec3356b8aeeaef52cb23437f475252789a0c3dd4acfa580a35e90a2b7f1a7cbf&start_rev=610713&end_rev=615636

Regressions are visible on 

x64, e.g. linux-perf
ia32, e.g. Win 7 Perf

Arm bots do not regress. An interesting side note is that they actually improve by 17% in a later follow-up CL that removes the DirectCEntry (and thus removes one indirection layer in CallApiCallback). This shows how sensitive these DOM benchmarks are even to minor CallApiCallback changes.


We do have options to optimize on ia32/x64:

1. Change the sub(rsp, _), mov(Operand(rsp, ...), ...) sequence back to a series of push instructions (https://cs.chromium.org/chromium/src/v8/src/builtins/x64/builtins-x64.cc?l=3216&rcl=146487a6764cd9ef5bd623eb828e9754fb09a5ba).
2. Set up part of the stack as part of the calling convention for CallApiCallback as described by this comment: https://cs.chromium.org/chromium/src/v8/src/builtins/x64/builtins-x64.cc?l=3156&rcl=146487a6764cd9ef5bd623eb828e9754fb09a5ba.

The caveat of option 1 is that consistency across architectures and readability is lost.
Option 2 would need a bit of brainpower (can callers push untagged args onto the stack?).


I'm not sure how to prioritize this. Jaro, Benedikt, thoughts?
The NextAction date has arrived: 2018-12-17
Cc: kouhei@chromium.org
 Issue 917229  has been merged into this issue.

Sign in to add a comment