dart-lang / sdk

The Dart SDK, including the VM, dart2js, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
9.97k stars 1.54k forks source link

GrowableList performance regression in 1.25.0-dev-3.0 #30090

Closed jcollins-g closed 7 years ago

jcollins-g commented 7 years ago

Hi,

After upgrading past 1.25.0-dev-2.0 I noticed dartdoc's overall runtime in checked mode nearly doubled. (non-checked mode runtime degrades by about 15%). This was pretty severe so I looked for a cause. The profiler suddenly had list expansion functions at the top of the list consistently:

screenshot from 2017-07-06 08 19 41

These were pretty much invisible before this version in the profiler.

The change:

https://github.com/dart-lang/sdk/commit/cec963f028198ec5871840491ba78b096ad0b819

seemed relevant to what I was seeing and so I tried reverting it, and performance returned to normal. Will mail a revert CL out soon.

jcollins-g commented 7 years ago

Revert CL: https://codereview.chromium.org/2968003004/

jcollins-g commented 7 years ago

A reproduction case:

$ git clone git@github.com:flutter/flutter.git
$ cd flutter
# Ignore errors, this is mostly to build the flutter tooling
$ bin/flutter analyze --flutter-repo
# This builds the docs with the Dart SDK set up for flutter (not what we want) but also sets up the fake package for dartdoc, which we need
$ dev/bots/docs.sh

# Now, run dartdoc very similarly to how flutter runs it:
$ cd dev/docs
$ dart --checked --enable-vm-service:40539 /usr/local/google/home/jcollins/dart/dartdoc/bin/dartdoc.dart --header styles.html --header analytics.html --footer-text lib/footer.html --exclude temp_doc --favicon=favicon.ico --use-categories --category-order "flutter,Dart Core,flutter_test,flutter_driver" --include-external flutter_test/lib/flutter_test.dart --include-external flutter_driver/lib/flutter_driver.dart --include-external flutter_driver/lib/driver_extension.dart --include-external flutter/lib/services.dart --include-external flutter/lib/material.dart --include-external flutter/lib/foundation.dart --include-external flutter/lib/rendering.dart --include-external flutter/lib/scheduler.dart --include-external flutter/lib/physics.dart --include-external flutter/lib/widgets.dart --include-external flutter/lib/painting.dart --include-external flutter/lib/animation.dart --include-external flutter/lib/gestures.dart --include-external flutter/lib/cupertino.dart --include-external platform_integration/lib/android.dart

Dartdoc's benchmarking will show a significant difference depending on whether the SDK was built with https://github.com/dart-lang/sdk/commit/cec963f028198ec5871840491ba78b096ad0b819.

a-siva commented 7 years ago

@mraleph @ErikCorryGoogle

askeksa-google commented 7 years ago

The excessive degradation in checked mode probably comes mainly from the new helper functions (_allocateData and _nextCapacity) which give rise to extra checks. When the code is optimized and these functions are inlined, their overhead is reduced, although it does look like there is still some overhead compared to inlining the code manually.

We could try and slice the growth strategy changes differently. Perhaps as a first step just add the odd-size strategy, which should be a definitive win (one more element can be added before each resize without affecting memory usage).

mraleph commented 7 years ago

This is caused by JIT optimizer being to optimistic (for its own good): see #30102 for the root cause.

We are going to land a workaround for this. We are not going to revert cec963f because it is just revealing an already existing issue.

mraleph commented 7 years ago

Thanks for a very good report and tracking it down @jcollins-g! d24040af02d13264c198ced6774c5ce32e75188a should fix / work around it so dartdoc performance should recover. Please let us know if it does not.