New issue
Advanced search Search tips

Issue 901747 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 15
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

1.1%-8.2% regression in memory.desktop at 604883:604916

Project Member Reported by jgruber@chromium.org, Nov 5

Issue description

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

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


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

linux-perf
mac-10_13_laptop_high_end-perf

memory.desktop - Benchmark documentation link:
  None

system_health.memory_desktop - Benchmark documentation link:
  https://bit.ly/system-health-benchmarks
Cc: ftang@chromium.org
Owner: ftang@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/131b0609e40000

[Intl] Move cachedOrNewService to C++ w/o caching by ftang@chromium.org
https://chromium.googlesource.com/v8/v8/+/dffaff7769dd89670a897d1adc50639b8655bf39
memory:chrome:all_processes:reported_by_chrome:v8:effective_size: 2.74e+06 → 2.907e+06 (+1.671e+05)

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

Benchmark documentation link:
  None
Cc: gsat...@chromium.org
It might be caused by create the instance over and over again before GC kick in.
Components: Blink>JavaScript>Internationalization
Labels: -Pri-2 OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows Pri-1
See for more discussion on this: 
https://bugs.chromium.org/p/chromium/issues/detail?id=901748#c7
Status: Started (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 13

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

commit 7aced29990db03006745ea0817ed23b3fd6d5760
Author: Frank Tang <ftang@chromium.org>
Date: Tue Nov 13 19:56:09 2018

[Intl] Cache intl objects in isolate

Remove old code in v8::Date::DateTimeConfigurationChangeNotification
Add code to clear date time cache object in isolate.

Running benchmark
python -u tools/run_perf.py --binary-override-path \
  out/x64.release/d8 --filter "JSTests/Strings/StringLocaleCompare" \
  test/js-perf-test/JSTests.json
python -u tools/run_perf.py --binary-override-path \
  out/x64.release/d8 --filter "JSTests/Dates" \
  test/js-perf-test/JSTests.json
python -u tools/run_perf.py --binary-override-path \
  out/x64.release/d8 --filter "JSTests/Numbers" \
  test/js-perf-test/JSTests.json

BEFORE THE FIX:
StringLocaleCompare-Strings(Score): 184287
toLocaleDateString-Dates(Score): 10456
toLocaleString-Dates(Score): 10436
toLocaleTimeString-Dates(Score): 10700
toLocaleString-Numbers(Score): 2935

AFTER THE FIX in Patch Set 13:
StringLocaleCompare-Strings(Score): 57470000
toLocaleDateString-Dates(Score): 6141000
toLocaleString-Dates(Score): 4093000
toLocaleTimeString-Dates(Score): 6323000
toLocaleString-Numbers(Score): 3371000

Bug: chromium:901748,  chromium:901747 ,  v8:5751 
Change-Id: I7578e2ced0fe967dce6424d17f15ab806cc522be
Reviewed-on: https://chromium-review.googlesource.com/c/1320892
Commit-Queue: Frank Tang <ftang@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57484}
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/api.cc
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/builtins/builtins-date.cc
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/builtins/builtins-intl.cc
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/isolate.cc
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/isolate.h
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/objects/intl-objects.cc
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/objects/intl-objects.h
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/objects/js-date-time-format.cc
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/objects/js-date-time-format.h
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/objects/js-number-format.cc
[modify] https://crrev.com/7aced29990db03006745ea0817ed23b3fd6d5760/src/objects/js-number-format.h

Status: Fixed (was: Started)
Status: Started (was: Fixed)
Have the graphs recovered? I'd like to wait until they do before closing this.

Looks like we're all recovered here. Nice work Frank!
Status: Fixed (was: Started)

Sign in to add a comment