Use explicit keys for often accessed closures

[kevmoo]

No description provided.

[jcollins-g]

Is hashing a string really noticeably faster than hashing a tearoff method?

[jcollins-g]

I'll mention that the point of the memoizer was to reduce boilerplate at call sites. This is adding more boilerplate. I'd like to see some evidence of a large performance win (say, at least 10% total runtime performance improvement outside of checked mode of flutter) before paying this cost.

[kevmoo]

I see over a doubling in speed. Check observatory when running – over half the time is calculating hashcode on Closures.

…

On Thu, Jan 11, 2018 at 9:02 AM, jcollins-g ***@***.***> wrote: I'll mention that the point of the memoizer was to reduce boilerplate at call sites. This is adding more boilerplate. I'd like to see some evidence of a large performance win (say, at least 10% total runtime performance improvement outside of checked mode of flutter) before paying this cost. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1591 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCiiM4sE6KRT4orUUon_IFOjtsTu9Dks5tJj6KgaJpZM4RaNgF> .

[kevmoo]

Last master build: 1130s for SDK gen https://travis-ci.org/dart-lang/dartdoc/jobs/327443617#L632
This PR: 520s for SDK gen https://travis-ci.org/dart-lang/dartdoc/jobs/327510156#L648

[jcollins-g]

It never occurred to me that a hashcode for a tearoff closure could take a non-trivial amount of time. But as you point out, it does seem to. I don't like the solution of introducing new keys though; perhaps in this instance we should just drop the caching.

But that brings up the larger question of whether the memoizer concept itself is even doable in Dart due to this issue. Just thinking here, but perhaps we should revert the memoizer application until that's determined?

[kevmoo]

perhaps we should revert the memoizer application until that's determined?

Maybe? An option that should be cheaper at runtime (but more code work) would be using source_gen to generate the boilerplate – but not sure it'd be worth it.

[jcollins-g]

I think what I'd like to do is to pretty much always pass a string key. I figured it might be a little slower but this is pretty ridiculously slow. Looks like the hashable list change is pretty valuable too, that's a sizeable chunk of the improvement even if it isn't most of it.

OK. Given the extraordinary impact of this change I'll defer my nits on whether I like some of the structure of it until later -- that'll motivate me to find a solution I like better and works for our users. :-) Approved.

[jcollins-g]

Adding @mraleph to this as Devon indicated he might be interested in why hashing tearoff closures is extremely slow.

[mraleph]

@jcollins-g

I have looked into the issue with hashCode performance for tear-offs and filed a bug based on what I saw, it is indeed far from optimal, see dart-lang/sdk#31875 for details.

One important thing to keep in mind about the line of code like:

String get fullyQualifiedNameWithoutLibrary =>
  _memoizer.memoized(_fullyQualifiedNameWithoutLibrary,
          altKey: 'fullyQualifiedNameWithoutLibrary');

is that VM allocates a new _fullyQualifiedNameWithoutLibrary tear-off every time you do e.fullyQualifiedNameWithoutLibrary. However VM does not allocate new string 'fullyQualifiedNameWithoutLibrary' - string literals are constants and are globally canonicalized.

So when you later do _fullyQualifiedNameWithoutLibrary.hashCode - VM needs to compute hashCode from scratch (because its a new tear-off instance there is no where to take cached hash code from). However 'fullyQualifiedNameWithoutLibrary'.hashCode is computed once and then it just returns cached hash code all the time, because it is the same string instance.

Now given how hot this code is I recommend to not use _memoizer here at all, considering how many objects you need to allocate every time fullyQualifiedNameWithoutLibrary is invoked.

My recommendation would be to do this:

diff --git a/lib/src/model.dart b/lib/src/model.dart
index f2c08702..f1f9deef 100644
--- a/lib/src/model.dart
+++ b/lib/src/model.dart
@@ -2917,14 +2917,16 @@ abstract class ModelElement extends Canonicalization

   String get fileName => "${name}.html";

+  String _fullyQualifiedName;
+  String _fullyQualifiedNameWithoutLibrary;
+
   /// Returns the fully qualified name.
   ///
   /// For example: libraryName.className.methodName
   @override
-  String get fullyQualifiedName => _memoizer.memoized(_buildFullyQualifiedName,
-      altKey: 'fullyQualifiedName');
+  String get fullyQualifiedName => _fullyQualifiedName ??= _buildFullyQualifiedName();

-  String _fullyQualifiedNameWithoutLibrary() {
+  String _buildFullyQualifiedNameWithoutLibrary() {
     // Remember, periods are legal in library names.
     return fullyQualifiedName.replaceFirst(
         "${library.fullyQualifiedName}.", '');
@@ -2934,8 +2936,7 @@ abstract class ModelElement extends Canonicalization
   // library.fullyQualifiedName seems somehow to break things.
   //String get fullyQualifiedNameWithoutLibrary => _memoizer.memoized2(fullyQualifiedName.replaceFirst, "${library.fullyQualifiedName}.", '');
   String get fullyQualifiedNameWithoutLibrary =>
-      _memoizer.memoized(_fullyQualifiedNameWithoutLibrary,
-          altKey: 'fullyQualifiedNameWithoutLibrary');
+      _fullyQualifiedNameWithoutLibrary ??= _buildFullyQualifiedNameWithoutLibrary();

   String get sourceFileName => element.source.fullName;

On my measurements it shaves away another ~80seconds from the docs generation time. I can send you this patch as a PR.

[Thanks for CCing me. Please don't hesitate to ask me any sort of performance questions! I am always happy to help with performance tuning. At the moment my capacity is a bit stretched due to Dart 2.0 migration - but after we get past that we can dig deeper into dartdoc performance on the VM]

[jcollins-g]

@mraleph Thanks for the look! Your suggestion is pretty much what this code used to do, but the combination of the VM allocating new tearoff objects and hashCode being slow surprised me (I've only been writing dart for a year or so).

I wrote the memoizer partially to clear up some boilerplate, but also to be able to add asserts later in checked mode for when things that were supposed to be static changed values underneath the cache. (See

dartdoc/lib/src/model_utils.dart

Line 225 in 52d46a4

if (!new DeepCollectionEquality()

for where I was going with that). That is a source of hard to understand bugs in dartdoc and is especially frustrating when I'm trying to make incremental changes to the design. I was willing to take some performance penalty for that opportunity to find more bugs -- just not as much as we wound up taking.

I think for now I'm going to revert the _memoizer change entirely; there's just too much of a hit to keep using it.