Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(263)

Issue 2988493002: Reapply "Improve hashCode for closure objects" with fixes. (Closed)

Created:
3 years, 5 months ago by alexmarkov
Modified:
3 years, 5 months ago
Reviewers:
zra, siva
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Reapply "Improve hashCode for closure objects" with fixes. This CL includes the following fixes: * Fix for incorrect non-nullable assumption about _Closure._hash field. * Add error handling into BecomeMapTraits::Hash. * Correct assertions for validating layout of Closure objects. * Add identityHashCode to the list of VM entry points in precompiler. Closes #30211. Original code review: https://codereview.chromium.org/2983823002/ Original CL description: This performance improvement is inspired by Flutter listeners stored in the HashSet (see ObserverList) and frequently checked using HashSet.contains(). If there are many such listeners and they are implicit instance closures (for example, created by 'new Listenable.merge(...)'), HashSet.contains() becomes very slow. It spends a lot of time in Closure_equals native method due to hash collisions between closure objects with same function but different receivers. This CL improves hashCode() calculation for implicit instance closures by mixing function hashcode with identity hashcode of the receiver. For explicit closures and static implicit closures hashCode() is improved by using identityHashCode() of a closure object. Also, hashcode is calculated once and cached in each closure instance. The size of a closure instance doesn't grow up because there was unused word-size padding both on 32-bit and 64-bit architectures. The execution time of the following micro-benchmark is reduced from 47665ms to 135ms on my Linux/x64 box. ------------------------------------- import "dart:collection"; class Foo { int _a; Foo(this._a); void bar() {} } main() { HashSet hs = new HashSet(); for (int i = 0; i < 1000; ++i) { hs.add(new Foo(i).bar); } var watch = new Stopwatch()..start(); for (int i = 0; i < 1000; ++i) { for (var c in hs) { hs.contains(c); } } int time = watch.elapsedMilliseconds; print("Time: ${time}ms\n"); } ------------------------------------- R=zra@google.com Committed: https://github.com/dart-lang/sdk/commit/203955bc48fdbd03b7b030c4e6dbcd20920201ec

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+109 lines, -40 lines) Patch
M runtime/lib/function.cc View 1 chunk +2 lines, -3 lines 0 comments Download
M runtime/lib/function.dart View 2 chunks +18 lines, -2 lines 0 comments Download
M runtime/vm/bootstrap.cc View 2 chunks +3 lines, -1 line 0 comments Download
M runtime/vm/bootstrap_natives.h View 1 chunk +1 line, -1 line 0 comments Download
M runtime/vm/bootstrap_nocore.cc View 2 chunks +3 lines, -1 line 0 comments Download
M runtime/vm/clustered_snapshot.cc View 1 chunk +0 lines, -1 line 0 comments Download
M runtime/vm/dart_entry.h View 1 chunk +3 lines, -0 lines 0 comments Download
M runtime/vm/dart_entry.cc View 1 chunk +18 lines, -0 lines 0 comments Download
M runtime/vm/isolate_reload.cc View 1 chunk +5 lines, -1 line 0 comments Download
M runtime/vm/object.h View 5 chunks +9 lines, -4 lines 0 comments Download
M runtime/vm/object.cc View 4 chunks +37 lines, -18 lines 0 comments Download
M runtime/vm/precompiler.cc View 1 chunk +1 line, -0 lines 0 comments Download
M runtime/vm/raw_object.h View 2 chunks +6 lines, -5 lines 0 comments Download
M runtime/vm/raw_object_snapshot.cc View 1 chunk +0 lines, -1 line 0 comments Download
M runtime/vm/symbols.h View 1 chunk +1 line, -0 lines 0 comments Download
M tests/language/language.status View 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 5 (2 generated)
alexmarkov
3 years, 5 months ago (2017-07-20 21:45:11 UTC) #2
zra
lgtm
3 years, 5 months ago (2017-07-20 21:55:51 UTC) #3
alexmarkov
3 years, 5 months ago (2017-07-20 22:22:25 UTC) #5
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as
203955bc48fdbd03b7b030c4e6dbcd20920201ec (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698