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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2332953002: Cache constructors of UnknownJavaScriptObject objects in side-map (Closed)

Created:
4 years, 3 months ago by sra1
Modified:
4 years ago
Reviewers:
Jacob
CC:
reviews_dartlang.org
Target Ref:
refs/heads/master
Visibility:
Public.

Description

(Replaced by https://codereview.chromium.org/2379173002 ) Cache constructors of UnknownJavaScriptObject objects in side-map No longer use constructor name to fix https://github.com/dart-lang/sdk/issues/25785 Faster lookup for dynamic JS interop on most objects (https://github.com/dart-lang/sdk/issues/26247) BUG=

Patch Set 1 #

Total comments: 2

Patch Set 2 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+147 lines, -16 lines) Patch
M sdk/lib/_internal/js_runtime/lib/interceptors.dart View 2 chunks +16 lines, -2 lines 0 comments Download
M sdk/lib/_internal/js_runtime/lib/native_helper.dart View 3 chunks +5 lines, -9 lines 0 comments Download
M tests/html/html.status View 1 2 chunks +4 lines, -1 line 0 comments Download
A tests/html/js_interop_constructor_name_test.dart View 1 chunk +98 lines, -0 lines 0 comments Download
A + tests/html/js_interop_constructor_name_test.html View 1 1 chunk +4 lines, -4 lines 0 comments Download
A tests/html/js_interop_constructor_name_test_js.js View 1 chunk +20 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (7 generated)
sra1
4 years, 3 months ago (2016-09-12 22:51:39 UTC) #7
Jacob
Excited to see this performance bottlekneck go away! https://codereview.chromium.org/2332953002/diff/60001/sdk/lib/_internal/js_runtime/lib/interceptors.dart File sdk/lib/_internal/js_runtime/lib/interceptors.dart (right): https://codereview.chromium.org/2332953002/diff/60001/sdk/lib/_internal/js_runtime/lib/interceptors.dart#newcode202 sdk/lib/_internal/js_runtime/lib/interceptors.dart:202: var ...
4 years, 3 months ago (2016-09-13 15:48:32 UTC) #8
Jacob
On 2016/09/13 15:48:32, Jacob wrote: > Excited to see this performance bottlekneck go away! > ...
4 years, 2 months ago (2016-09-26 16:55:48 UTC) #9
kevmoo
On 2016/09/26 16:55:48, Jacob wrote: > On 2016/09/13 15:48:32, Jacob wrote: > > Excited to ...
4 years, 2 months ago (2016-09-27 17:41:27 UTC) #10
sra1
I think it is OK to proceed with the WeakMap and investigate a property later. ...
4 years, 2 months ago (2016-09-28 17:33:00 UTC) #11
Jacob
4 years, 2 months ago (2016-09-28 17:36:39 UTC) #12 
On 2016/09/28 17:33:00, sra1 wrote:
> I think it is OK to proceed with the WeakMap and investigate a property later.
> 
> What remains to be done is figure out how to fix all >100 dart_native tests
> (since the functions no longer are 'native') and then fix them all.
> 
>
https://codereview.chromium.org/2332953002/diff/60001/sdk/lib/_internal/js_ru...
> File sdk/lib/_internal/js_runtime/lib/interceptors.dart (right):
> 
>
https://codereview.chromium.org/2332953002/diff/60001/sdk/lib/_internal/js_ru...
> sdk/lib/_internal/js_runtime/lib/interceptors.dart:202: var
> constructorToInterceptor =
> On 2016/09/13 15:48:32, Jacob wrote:
> > wouldn't it be significantly faster to cache interceptors using a symbol
> stored
> > on the constructor (where possible) instead of a WeakMap? 
> 
> It is faster on hits and slower on misses.
> There have historically been problems putting properties on browser objects,
> prototypes and constructors or some browser. (This would apply to objects that
> are not part of the IDL). I don't know if they are still issues.

Placing symbols on browser objects is completely safe. Agree that you have to be
pretty careful adding properties particularly if the properties are enumerable.
I'm fine with landing with WeakMap.

lgtm

Lgtm

Powered by Google App Engine
This is Rietveld 408576698