Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
282893002:
Use originalGetTag on wrapped object (Closed)
|
Created:
6 years, 7 months ago by sra1 Modified:
6 years, 7 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionUse originalGetTag on wrapped object
Upgrade dart_support to use dartNativeDispatchHooksTransformer.
R=sigmund@google.com
Committed: https://code.google.com/p/dart/source/detail?r=36135
Patch Set 1 : #
Total comments: 4
Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... File pkg/web_components/lib/dart_support.js (right): https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... pkg/web_components/lib/dart_support.js:54: name = originalGetTag(unwrapped); Interesting! Do we still need ctor._ShadowDOMPolyfill$cacheTag_ ? It seems like we're willing to `return originalGetTag(obj);` below so I'm guessing it's not needed. We could replace everything starting from line 52 to 57 (in the new code) with just `return originalGetTag(unwrapped);` I'm not sure if that then allows even more code folding with line 62 or not... anyway, cool to see the Firefox code needs less workarounds now!
thanks Stephen! https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... File pkg/web_components/lib/dart_support.js (right): https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... pkg/web_components/lib/dart_support.js:54: name = originalGetTag(unwrapped); On 2014/05/13 21:01:31, John Messerly wrote: > Interesting! Do we still need ctor._ShadowDOMPolyfill$cacheTag_ ? It seems like > we're willing to `return originalGetTag(obj);` below so I'm guessing it's not > needed. We could replace everything starting from line 52 to 57 (in the new > code) with just `return originalGetTag(unwrapped);` > > I'm not sure if that then allows even more code folding with line 62 or not... > > anyway, cool to see the Firefox code needs less workarounds now! > +1, lgtm, if we don't need the caching, I'm happy to get rid of it too :)
Message was sent while issue was closed.
Committed patchset #1 manually as r36135 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... File pkg/web_components/lib/dart_support.js (right): https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... pkg/web_components/lib/dart_support.js:54: name = originalGetTag(unwrapped); On 2014/05/13 21:06:58, Siggi Cherem (dart-lang) wrote: > On 2014/05/13 21:01:31, John Messerly wrote: > > Interesting! Do we still need ctor._ShadowDOMPolyfill$cacheTag_ ? It seems > like > > we're willing to `return originalGetTag(obj);` below so I'm guessing it's not > > needed. We could replace everything starting from line 52 to 57 (in the new > > code) with just `return originalGetTag(unwrapped);` > > > > I'm not sure if that then allows even more code folding with line 62 or not... > > > > anyway, cool to see the Firefox code needs less workarounds now! > > > > +1, lgtm, if we don't need the caching, I'm happy to get rid of it too :) FYI - we chatted more about it offline and decided to keep the cashing for now (even though we removed the toString here, it is still used underneath, so caching it might not be a bad idea, we can measure and revisit if we want later).
Message was sent while issue was closed.
https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... File pkg/web_components/lib/dart_support.js (right): https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... pkg/web_components/lib/dart_support.js:54: name = originalGetTag(unwrapped); On 2014/05/13 21:55:21, Siggi Cherem (dart-lang) wrote: > On 2014/05/13 21:06:58, Siggi Cherem (dart-lang) wrote: > > On 2014/05/13 21:01:31, John Messerly wrote: > > > Interesting! Do we still need ctor._ShadowDOMPolyfill$cacheTag_ ? It seems > > like > > > we're willing to `return originalGetTag(obj);` below so I'm guessing it's > not > > > needed. We could replace everything starting from line 52 to 57 (in the new > > > code) with just `return originalGetTag(unwrapped);` > > > > > > I'm not sure if that then allows even more code folding with line 62 or > not... > > > > > > anyway, cool to see the Firefox code needs less workarounds now! > > > > > > > +1, lgtm, if we don't need the caching, I'm happy to get rid of it too :) > > FYI - we chatted more about it offline and decided to keep the cashing for now > (even though we removed the toString here, it is still used underneath, so > caching it might not be a bad idea, we can measure and revisit if we want > later). interesting. yeah, seems okay, but makes me wonder why we don't cache for everything then? FWIW -- I thought in the old dart2js code, "originalGetTag" did cache. At least, that was my understanding when I reviewed the dart2js code at the time, and reason it does not attempt caching below.
Message was sent while issue was closed.
On 2014/05/13 22:12:36, John Messerly wrote: > https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... > File pkg/web_components/lib/dart_support.js (right): > > https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... > pkg/web_components/lib/dart_support.js:54: name = originalGetTag(unwrapped); > On 2014/05/13 21:55:21, Siggi Cherem (dart-lang) wrote: > > On 2014/05/13 21:06:58, Siggi Cherem (dart-lang) wrote: > > > On 2014/05/13 21:01:31, John Messerly wrote: > > > > Interesting! Do we still need ctor._ShadowDOMPolyfill$cacheTag_ ? It seems > > > like > > > > we're willing to `return originalGetTag(obj);` below so I'm guessing it's > > not > > > > needed. We could replace everything starting from line 52 to 57 (in the > new > > > > code) with just `return originalGetTag(unwrapped);` > > > > > > > > I'm not sure if that then allows even more code folding with line 62 or > > not... > > > > > > > > anyway, cool to see the Firefox code needs less workarounds now! > > > > > > > > > > +1, lgtm, if we don't need the caching, I'm happy to get rid of it too :) > > > > FYI - we chatted more about it offline and decided to keep the cashing for now > > (even though we removed the toString here, it is still used underneath, so > > caching it might not be a bad idea, we can measure and revisit if we want > > later). > > interesting. yeah, seems okay, but makes me wonder why we don't cache for > everything then? > > FWIW -- I thought in the old dart2js code, "originalGetTag" did cache. At least, > that was my understanding when I reviewed the dart2js code at the time, and > reason it does not attempt caching below. in other words -- because I was using toString and directly bypassing "originalGetTag" I needed my own cache. At least that original line of thought doesn't apply :) ... but maybe there are valid reasons to have a cache here on our side.
Message was sent while issue was closed.
On 2014/05/13 22:13:58, John Messerly wrote: > On 2014/05/13 22:12:36, John Messerly wrote: > > > https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... > > File pkg/web_components/lib/dart_support.js (right): > > > > > https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... > > pkg/web_components/lib/dart_support.js:54: name = originalGetTag(unwrapped); > > On 2014/05/13 21:55:21, Siggi Cherem (dart-lang) wrote: > > > On 2014/05/13 21:06:58, Siggi Cherem (dart-lang) wrote: > > > > On 2014/05/13 21:01:31, John Messerly wrote: > > > > > Interesting! Do we still need ctor._ShadowDOMPolyfill$cacheTag_ ? It > seems > > > > like > > > > > we're willing to `return originalGetTag(obj);` below so I'm guessing > it's > > > not > > > > > needed. We could replace everything starting from line 52 to 57 (in the > > new > > > > > code) with just `return originalGetTag(unwrapped);` > > > > > > > > > > I'm not sure if that then allows even more code folding with line 62 or > > > not... > > > > > > > > > > anyway, cool to see the Firefox code needs less workarounds now! > > > > > > > > > > > > > +1, lgtm, if we don't need the caching, I'm happy to get rid of it too :) > > > > > > FYI - we chatted more about it offline and decided to keep the cashing for > now > > > (even though we removed the toString here, it is still used underneath, so > > > caching it might not be a bad idea, we can measure and revisit if we want > > > later). > > > > interesting. yeah, seems okay, but makes me wonder why we don't cache for > > everything then? > > > > FWIW -- I thought in the old dart2js code, "originalGetTag" did cache. At > least, > > that was my understanding when I reviewed the dart2js code at the time, and > > reason it does not attempt caching below. > > in other words -- because I was using toString and directly bypassing > "originalGetTag" I needed my own cache. At least that original line of thought > doesn't apply :) ... but maybe there are valid reasons to have a cache here on > our side. Ah, good point! the cache is definitely there, so we should be able to remove it here.
Message was sent while issue was closed.
On 2014/05/13 22:27:41, Siggi Cherem (dart-lang) wrote: > On 2014/05/13 22:13:58, John Messerly wrote: > > On 2014/05/13 22:12:36, John Messerly wrote: > > > > > > https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... > > > File pkg/web_components/lib/dart_support.js (right): > > > > > > > > > https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... > > > pkg/web_components/lib/dart_support.js:54: name = originalGetTag(unwrapped); > > > On 2014/05/13 21:55:21, Siggi Cherem (dart-lang) wrote: > > > > On 2014/05/13 21:06:58, Siggi Cherem (dart-lang) wrote: > > > > > On 2014/05/13 21:01:31, John Messerly wrote: > > > > > > Interesting! Do we still need ctor._ShadowDOMPolyfill$cacheTag_ ? It > > seems > > > > > like > > > > > > we're willing to `return originalGetTag(obj);` below so I'm guessing > > it's > > > > not > > > > > > needed. We could replace everything starting from line 52 to 57 (in > the > > > new > > > > > > code) with just `return originalGetTag(unwrapped);` > > > > > > > > > > > > I'm not sure if that then allows even more code folding with line 62 > or > > > > not... > > > > > > > > > > > > anyway, cool to see the Firefox code needs less workarounds now! > > > > > > > > > > > > > > > > +1, lgtm, if we don't need the caching, I'm happy to get rid of it too > :) > > > > > > > > FYI - we chatted more about it offline and decided to keep the cashing for > > now > > > > (even though we removed the toString here, it is still used underneath, so > > > > caching it might not be a bad idea, we can measure and revisit if we want > > > > later). > > > > > > interesting. yeah, seems okay, but makes me wonder why we don't cache for > > > everything then? > > > > > > FWIW -- I thought in the old dart2js code, "originalGetTag" did cache. At > > least, > > > that was my understanding when I reviewed the dart2js code at the time, and > > > reason it does not attempt caching below. > > > > in other words -- because I was using toString and directly bypassing > > "originalGetTag" I needed my own cache. At least that original line of thought > > doesn't apply :) ... but maybe there are valid reasons to have a cache here on > > our side. > > Ah, good point! the cache is definitely there, so we should be able to remove it > here. Where is the cache that is 'definitely there'? The caching is (should be) above this level. getTag has never cached. The result of getTag determines if a dispatch record containing the interceptor can be cached somewhere to avoid future lookup. This is one reason why originalGetTag must be called - everyone gets to vote on avoiding caching. getTag returns "Foo" -> dispatch record will be cached on the prototype and getTag should never be called again for this class getTag returns "!Foo" -> dispatch record is cached on the instance, getTag is called once per instance getTag returns "~Foo" -> dispatch record is never cached, getTag is called once per "operation", with some reduction via localized common subexpression elimination. The caching in this file would be useful when there are caching limitations. Caching limitations may be added in future to work around browser issues, e.g. we use "!Location" to avoid touching the location prototype which is poisonous. Ideally the code here would be concerned only with unwrapping the objects that web_components wraps and nothing else. With my other CL we might be getting close.
Message was sent while issue was closed.
On 2014/05/13 22:27:41, Siggi Cherem (dart-lang) wrote: > On 2014/05/13 22:13:58, John Messerly wrote: > > On 2014/05/13 22:12:36, John Messerly wrote: > > > > > > https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... > > > File pkg/web_components/lib/dart_support.js (right): > > > > > > > > > https://codereview.chromium.org/282893002/diff/80001/pkg/web_components/lib/d... > > > pkg/web_components/lib/dart_support.js:54: name = originalGetTag(unwrapped); > > > On 2014/05/13 21:55:21, Siggi Cherem (dart-lang) wrote: > > > > On 2014/05/13 21:06:58, Siggi Cherem (dart-lang) wrote: > > > > > On 2014/05/13 21:01:31, John Messerly wrote: > > > > > > Interesting! Do we still need ctor._ShadowDOMPolyfill$cacheTag_ ? It > > seems > > > > > like > > > > > > we're willing to `return originalGetTag(obj);` below so I'm guessing > > it's > > > > not > > > > > > needed. We could replace everything starting from line 52 to 57 (in > the > > > new > > > > > > code) with just `return originalGetTag(unwrapped);` > > > > > > > > > > > > I'm not sure if that then allows even more code folding with line 62 > or > > > > not... > > > > > > > > > > > > anyway, cool to see the Firefox code needs less workarounds now! > > > > > > > > > > > > > > > > +1, lgtm, if we don't need the caching, I'm happy to get rid of it too > :) > > > > > > > > FYI - we chatted more about it offline and decided to keep the cashing for > > now > > > > (even though we removed the toString here, it is still used underneath, so > > > > caching it might not be a bad idea, we can measure and revisit if we want > > > > later). > > > > > > interesting. yeah, seems okay, but makes me wonder why we don't cache for > > > everything then? > > > > > > FWIW -- I thought in the old dart2js code, "originalGetTag" did cache. At > > least, > > > that was my understanding when I reviewed the dart2js code at the time, and > > > reason it does not attempt caching below. > > > > in other words -- because I was using toString and directly bypassing > > "originalGetTag" I needed my own cache. At least that original line of thought > > doesn't apply :) ... but maybe there are valid reasons to have a cache here on > > our side. > > Ah, good point! the cache is definitely there, so we should be able to remove it > here. Where is the cache that is 'definitely there'? The caching is (should be) above this level. getTag has never cached. The result of getTag determines if a dispatch record containing the interceptor can be cached somewhere to avoid future lookup. This is one reason why originalGetTag must be called - everyone gets to vote on avoiding caching. getTag returns "Foo" -> dispatch record will be cached on the prototype and getTag should never be called again for this class getTag returns "!Foo" -> dispatch record is cached on the instance, getTag is called once per instance getTag returns "~Foo" -> dispatch record is never cached, getTag is called once per "operation", with some reduction via localized common subexpression elimination. The caching in this file would be useful when there are caching limitations. Caching limitations may be added in future to work around browser issues, e.g. we use "!Location" to avoid touching the location prototype which is poisonous. Ideally the code here would be concerned only with unwrapping the objects that web_components wraps and nothing else. With my other CL we might be getting close.
Message was sent while issue was closed.
I just realized - we should fix this to include a pubspec.yaml change including and new SDK constraint |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
