Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
974803002:
Defer addStubs to class instantiation time. (Closed)
|
Created:
5 years, 9 months ago by herhut Modified:
5 years, 9 months ago CC:
reviews_dartlang.org, ahe, zarah Target Ref:
refs/remotes/git-svn Visibility:
Public. |
DescriptionDefer addStubs to class instantiation time.
BUG=
R=floitsch@google.com
Committed: https://code.google.com/p/dart/source/detail?r=44458
Patch Set 1 #
Total comments: 1
Patch Set 2 : Ensure fast prototypes and avoid polymorphic access in constructor #
Total comments: 39
Patch Set 3 : Comments #Patch Set 4 : Comments #Patch Set 5 : rebase #Patch Set 6 : Use JS_NAME and fix deferred loading. #
Total comments: 32
Patch Set 7 : More comments #Patch Set 8 : Rebased and fixes. #Messages
Total messages: 16 (2 generated)
herhut@google.com changed reviewers: + floitsch@google.com
This needs some more performance tuning but is functionally complete (as in passes all tests).
PTAL This now passes all the performance tests without major regressions and some improvements. So this is ready to go.
sra@google.com changed reviewers: + sra@google.com
DBC. I can't git cl patch. What is your base revision? https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/ssa... File pkg/compiler/lib/src/ssa/codegen_helpers.dart (right): https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/ssa... pkg/compiler/lib/src/ssa/codegen_helpers.dart:399: // after first access if we use lazy initialization. This requires inlining of method, right? new A().m() where m() => super.m() https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:1677: if (JS('bool', '!!#.\$deferredAction', prototype)) { Other places use the namer. Perhaps use a different name in minified code to test for this. https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/js_lib/js_names.dart (right): https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/js_lib/js_names.dart:61: JS('bool', '# == null || typeof # !== "string"', result, result); typeof is sufficient
This requires a couple of other cls: https://codereview.chromium.org/946023004/ https://codereview.chromium.org/961653002/ https://codereview.chromium.org/948383003/ https://codereview.chromium.org/957343002/ https://codereview.chromium.org/963453002/ and they are all based on a weeks old version of dart2js. I will start landing those soon. https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/ssa... File pkg/compiler/lib/src/ssa/codegen_helpers.dart (right): https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/ssa... pkg/compiler/lib/src/ssa/codegen_helpers.dart:399: // after first access if we use lazy initialization. On 2015/03/05 18:23:25, sra1 wrote: > This requires inlining of method, right? > > new A().m() where m() => super.m() Yes. We have an existing test that does this. https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:1677: if (JS('bool', '!!#.\$deferredAction', prototype)) { On 2015/03/05 18:23:25, sra1 wrote: > Other places use the namer. > Perhaps use a different name in minified code to test for this. Thanks for catching this. I will have it go through JS_GET_NAME. https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/js_lib/js_names.dart (right): https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/js_lib/js_names.dart:61: JS('bool', '# == null || typeof # !== "string"', result, result); On 2015/03/05 18:23:25, sra1 wrote: > typeof is sufficient True, thanks!
LGTM. why is the js_names change required in this CL? https://codereview.chromium.org/974803002/diff/1/pkg/compiler/lib/src/js_emit... File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right): https://codereview.chromium.org/974803002/diff/1/pkg/compiler/lib/src/js_emit... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:554: // first instantiating them first. delete first "first". https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/js_... File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right): https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:338: if (supportsDirectProtoAccess) Sigurd put in an optimization that removes blocks if they only contain one statement. -> add the curly braces. https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:529: prototype.#deferredAction = markerFun; given that this function comes from a different file, use a shared constant to link them. https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:529: prototype.#deferredAction = markerFun; Explain what the marker-fun is for (either here or at the marker-fun declaration. maybe both). https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:547: if (constructor.prototype.#deferredAction) ditto (add curly braces). https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:551: // first instantiating them first. This looks like we are only eagerly running the deferredAction if the direct superclass is the interceptor class. Are we sure that there are not sub-subclasses? I'm particularly worried about typed-arrays. https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1158: jsAst.Statement supportsDirectProtoAccess = js.statement(r''' Since this is used in a completely different part of the code, use a common constant to link the places. Maybe we should have a class: ClassConstructionNames constructionNames; and then use it with constructionNames.supportsDirectProtoAccess. This way we wouldn't have all these "final XyzName = '...'" and group them into nice classes. Ideally we should then run them through the namer to make sure that they are safe (and don't shadow anything important). https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/js_... File pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart (right): https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:72: firstChar !== 43 && // 43 is aka "+". Please add comment what this is for. The "+" and "*" are probably more misleading to new readers. https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:98: delete descriptor.static; we don't need it for reflection? https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/ssa... File pkg/compiler/lib/src/ssa/codegen_helpers.dart (right): https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/ssa... pkg/compiler/lib/src/ssa/codegen_helpers.dart:394: // C.prototype.method.call(instance) indent by 4. https://codereview.chromium.org/974803002/diff/20001/pkg/compiler/lib/src/ssa... pkg/compiler/lib/src/ssa/codegen_helpers.dart:397: // get of prototype might be evaluted before instance is created if we evaluated https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... File sdk/lib/_internal/compiler/js_lib/js_names.dart (right): https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/js_lib/js_names.dart:70: var result = super[key]; Use type. https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/js_lib/js_names.dart:72: if (result == null && key.startsWith(setterPrefix)) { Isn't this handled by the "_updateReflectiveNames below? If not, explain why not. https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/js_lib/js_names.dart:79: return (result != null) ? "${result}=" : null; You don't need the curly braces. https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/js_lib/js_names.dart:88: var _cacheLength = 0; types. https://codereview.chromium.org/974803002/diff/20001/sdk/lib/_internal/compil... sdk/lib/_internal/compiler/js_lib/js_names.dart:95: var result = <String, String>{}; Use types.
https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right): https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:338: if (supportsDirectProtoAccess) On 2015/03/06 14:54:09, floitsch wrote: > Sigurd put in an optimization that removes blocks if they only contain one > statement. > -> add the curly braces. WHOOHOO! Done. https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:529: prototype.#deferredAction = markerFun; On 2015/03/06 14:54:09, floitsch wrote: > given that this function comes from a different file, use a shared constant to > link them. zarah@ is working on putting the two chunks of JS into one file, so she would undo it soon, anyway. I'll leave it as it is. https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:529: prototype.#deferredAction = markerFun; On 2015/03/06 14:54:09, floitsch wrote: > Explain what the marker-fun is for (either here or at the marker-fun > declaration. maybe both). Done. https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:547: if (constructor.prototype.#deferredAction) On 2015/03/06 14:54:09, floitsch wrote: > ditto (add curly braces). Done. https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1158: jsAst.Statement supportsDirectProtoAccess = js.statement(r''' On 2015/03/06 14:54:09, floitsch wrote: > Since this is used in a completely different part of the code, use a common > constant to link the places. > > Maybe we should have a class: ClassConstructionNames constructionNames; > and then use it with constructionNames.supportsDirectProtoAccess. > > This way we wouldn't have all these "final XyzName = '...'" and group them into > nice classes. > Ideally we should then run them through the namer to make sure that they are > safe (and don't shadow anything important). I would actually prefer to emit this as part of the combined reflectionDataParser with finishClasses, as this should not be visible to other parts of the code. It currently is here as we have this artificial split between finishClasses and parsing reflection data. I will work with zarah@ to make this happen in a follow up CL. https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... File pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart (right): https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:72: firstChar !== 43 && // 43 is aka "+". On 2015/03/06 14:54:09, floitsch wrote: > Please add comment what this is for. The "+" and "*" are probably more > misleading to new readers. Done. https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:98: delete descriptor.static; On 2015/03/06 14:54:09, floitsch wrote: > we don't need it for reflection? No, statics are collected separately in an array that is then used by the mirrors runtime. https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... File pkg/compiler/lib/src/ssa/codegen_helpers.dart (right): https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/ssa/codegen_helpers.dart:394: // C.prototype.method.call(instance) On 2015/03/06 14:54:10, floitsch wrote: > indent by 4. Done. https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/ssa/codegen_helpers.dart:397: // get of prototype might be evaluted before instance is created if we On 2015/03/06 14:54:09, floitsch wrote: > evaluated Thanks! https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... File sdk/lib/_internal/compiler/js_lib/js_names.dart (right): https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... sdk/lib/_internal/compiler/js_lib/js_names.dart:70: var result = super[key]; On 2015/03/06 14:54:10, floitsch wrote: > Use type. Done. https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... sdk/lib/_internal/compiler/js_lib/js_names.dart:72: if (result == null && key.startsWith(setterPrefix)) { On 2015/03/06 14:54:10, floitsch wrote: > Isn't this handled by the "_updateReflectiveNames below? > If not, explain why not. _updateReflectiveNames is the inverse mapping. This is for mapping from mangled names to reflective (or unmangled) names. The _updateReflectiveNames computes a mapping from reflective (unmangled) names back to mangled names. https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... sdk/lib/_internal/compiler/js_lib/js_names.dart:79: return (result != null) ? "${result}=" : null; On 2015/03/06 14:54:10, floitsch wrote: > You don't need the curly braces. I actually find this easier to read. Unless you really mind, I would prefer it like this. https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... sdk/lib/_internal/compiler/js_lib/js_names.dart:88: var _cacheLength = 0; On 2015/03/06 14:54:10, floitsch wrote: > types. Done. https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... sdk/lib/_internal/compiler/js_lib/js_names.dart:95: var result = <String, String>{}; On 2015/03/06 14:54:10, floitsch wrote: > Use types. Done.
[+ zarah@] On 2015/03/09 14:28:35, herhut wrote: > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right): > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:338: if > (supportsDirectProtoAccess) > On 2015/03/06 14:54:09, floitsch wrote: > > Sigurd put in an optimization that removes blocks if they only contain one > > statement. > > -> add the curly braces. > > WHOOHOO! > > Done. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:529: > prototype.#deferredAction = markerFun; > On 2015/03/06 14:54:09, floitsch wrote: > > given that this function comes from a different file, use a shared constant to > > link them. > > zarah@ is working on putting the two chunks of JS into one file, so she would > undo it soon, anyway. I'll leave it as it is. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:529: > prototype.#deferredAction = markerFun; > On 2015/03/06 14:54:09, floitsch wrote: > > Explain what the marker-fun is for (either here or at the marker-fun > > declaration. maybe both). > > Done. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:547: if > (constructor.prototype.#deferredAction) > On 2015/03/06 14:54:09, floitsch wrote: > > ditto (add curly braces). > > Done. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1158: jsAst.Statement > supportsDirectProtoAccess = js.statement(r''' > On 2015/03/06 14:54:09, floitsch wrote: > > Since this is used in a completely different part of the code, use a common > > constant to link the places. > > > > Maybe we should have a class: ClassConstructionNames constructionNames; > > and then use it with constructionNames.supportsDirectProtoAccess. > > > > This way we wouldn't have all these "final XyzName = '...'" and group them > into > > nice classes. > > Ideally we should then run them through the namer to make sure that they are > > safe (and don't shadow anything important). > > I would actually prefer to emit this as part of the combined > reflectionDataParser with finishClasses, as this should not be visible to other > parts of the code. It currently is here as we have this artificial split between > finishClasses and parsing reflection data. > > I will work with zarah@ to make this happen in a follow up CL. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > File pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart > (right): > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:72: > firstChar !== 43 && // 43 is aka "+". > On 2015/03/06 14:54:09, floitsch wrote: > > Please add comment what this is for. The "+" and "*" are probably more > > misleading to new readers. > > Done. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:98: > delete descriptor.static; > On 2015/03/06 14:54:09, floitsch wrote: > > we don't need it for reflection? > > No, statics are collected separately in an array that is then used by the > mirrors runtime. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > File pkg/compiler/lib/src/ssa/codegen_helpers.dart (right): > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > pkg/compiler/lib/src/ssa/codegen_helpers.dart:394: // > C.prototype.method.call(instance) > On 2015/03/06 14:54:10, floitsch wrote: > > indent by 4. > > Done. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > pkg/compiler/lib/src/ssa/codegen_helpers.dart:397: // get of prototype might be > evaluted before instance is created if we > On 2015/03/06 14:54:09, floitsch wrote: > > evaluated > > Thanks! > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > File sdk/lib/_internal/compiler/js_lib/js_names.dart (right): > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > sdk/lib/_internal/compiler/js_lib/js_names.dart:70: var result = super[key]; > On 2015/03/06 14:54:10, floitsch wrote: > > Use type. > > Done. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > sdk/lib/_internal/compiler/js_lib/js_names.dart:72: if (result == null && > key.startsWith(setterPrefix)) { > On 2015/03/06 14:54:10, floitsch wrote: > > Isn't this handled by the "_updateReflectiveNames below? > > If not, explain why not. > > _updateReflectiveNames is the inverse mapping. This is for mapping from mangled > names to reflective (or unmangled) names. The _updateReflectiveNames computes a > mapping from reflective (unmangled) names back to mangled names. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > sdk/lib/_internal/compiler/js_lib/js_names.dart:79: return (result != null) ? > "${result}=" : null; > On 2015/03/06 14:54:10, floitsch wrote: > > You don't need the curly braces. > > I actually find this easier to read. Unless you really mind, I would prefer it > like this. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > sdk/lib/_internal/compiler/js_lib/js_names.dart:88: var _cacheLength = 0; > On 2015/03/06 14:54:10, floitsch wrote: > > types. > > Done. > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > sdk/lib/_internal/compiler/js_lib/js_names.dart:95: var result = <String, > String>{}; > On 2015/03/06 14:54:10, floitsch wrote: > > Use types. > > Done.
https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right): https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:551: // first instantiating them first. On 2015/03/06 14:54:09, floitsch wrote: > This looks like we are only eagerly running the deferredAction if the direct > superclass is the interceptor class. > Are we sure that there are not sub-subclasses? I'm particularly worried about > typed-arrays. any thoughts on this one? https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... File sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:1677: if (JS('bool', '!!#.\$deferredAction', prototype)) { On 2015/03/06 12:36:45, herhut wrote: > On 2015/03/05 18:23:25, sra1 wrote: > > Other places use the namer. > > Perhaps use a different name in minified code to test for this. > > Thanks for catching this. I will have it go through JS_GET_NAME. not done yet. https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... File sdk/lib/_internal/compiler/js_lib/js_names.dart (right): https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... sdk/lib/_internal/compiler/js_lib/js_names.dart:72: if (result == null && key.startsWith(setterPrefix)) { On 2015/03/09 14:28:35, herhut wrote: > On 2015/03/06 14:54:10, floitsch wrote: > > Isn't this handled by the "_updateReflectiveNames below? > > If not, explain why not. > > _updateReflectiveNames is the inverse mapping. This is for mapping from mangled > names to reflective (or unmangled) names. The _updateReflectiveNames computes a > mapping from reflective (unmangled) names back to mangled names. Explain in a comment. https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... sdk/lib/_internal/compiler/js_lib/js_names.dart:74: int setterPrefixLength = getterPrefix.length; getterPrefixLength
On 2015/03/09 16:35:06, floitsch wrote: > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right): > > https://chromiumcodereview.appspot.com/974803002/diff/20001/pkg/compiler/lib/... > pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:551: // first > instantiating them first. > On 2015/03/06 14:54:09, floitsch wrote: > > This looks like we are only eagerly running the deferredAction if the direct > > superclass is the interceptor class. > > Are we sure that there are not sub-subclasses? I'm particularly worried about > > typed-arrays. > > any thoughts on this one? Yes, you were right. JSDouble -> JSInt -> JSNumber -> Interceptor is an example. I now use "isInterceptor" to check whether the Interceptor class is somewhere on the inheritance chain. > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > File sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:1677: if (JS('bool', > '!!#.\$deferredAction', prototype)) { > On 2015/03/06 12:36:45, herhut wrote: > > On 2015/03/05 18:23:25, sra1 wrote: > > > Other places use the namer. > > > Perhaps use a different name in minified code to test for this. > > > > Thanks for catching this. I will have it go through JS_GET_NAME. > > not done yet. I need to do a big rebase so I get the enum version of JS_GET_NAME to do this. I'll do that now. > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > File sdk/lib/_internal/compiler/js_lib/js_names.dart (right): > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > sdk/lib/_internal/compiler/js_lib/js_names.dart:72: if (result == null && > key.startsWith(setterPrefix)) { > On 2015/03/09 14:28:35, herhut wrote: > > On 2015/03/06 14:54:10, floitsch wrote: > > > Isn't this handled by the "_updateReflectiveNames below? > > > If not, explain why not. > > > > _updateReflectiveNames is the inverse mapping. This is for mapping from > mangled > > names to reflective (or unmangled) names. The _updateReflectiveNames computes > a > > mapping from reflective (unmangled) names back to mangled names. > > Explain in a comment. Done.
This is now good to go. On 2015/03/10 13:35:26, herhut wrote: > On 2015/03/09 16:35:06, floitsch wrote: > > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > > File sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): > > > > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > > sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:1677: if (JS('bool', > > '!!#.\$deferredAction', prototype)) { > > On 2015/03/06 12:36:45, herhut wrote: > > > On 2015/03/05 18:23:25, sra1 wrote: > > > > Other places use the namer. > > > > Perhaps use a different name in minified code to test for this. > > > > > > Thanks for catching this. I will have it go through JS_GET_NAME. > > > > not done yet. > > I need to do a big rebase so I get the enum version of JS_GET_NAME to do this. > I'll do that now. > Done now.
Oh, and PTAL again. On 2015/03/10 16:00:03, herhut wrote: > This is now good to go. > > On 2015/03/10 13:35:26, herhut wrote: > > On 2015/03/09 16:35:06, floitsch wrote: > > > > > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > > > File sdk/lib/_internal/compiler/js_lib/js_mirrors.dart (right): > > > > > > > > > https://chromiumcodereview.appspot.com/974803002/diff/20001/sdk/lib/_internal... > > > sdk/lib/_internal/compiler/js_lib/js_mirrors.dart:1677: if (JS('bool', > > > '!!#.\$deferredAction', prototype)) { > > > On 2015/03/06 12:36:45, herhut wrote: > > > > On 2015/03/05 18:23:25, sra1 wrote: > > > > > Other places use the namer. > > > > > Perhaps use a different name in minified code to test for this. > > > > > > > > Thanks for catching this. I will have it go through JS_GET_NAME. > > > > > > not done yet. > > > > I need to do a big rebase so I get the enum version of JS_GET_NAME to do this. > > I'll do that now. > > > > Done now.
Still LGTM. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right): https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:401: }''', { 'deferredAction': js.string(namer.deferredAction), potentially create the string here? (although maybe not worth it). that is: body += #deferredActionCall 'deferredActionCall': js.string("this.${namer.deferredAction}()") https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart (right): https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:59: var prototype = prototype || this; document this. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:94: var deferWork = supportsDirectProtoAccess && cls != #objectClassName; shouldDeferWork ? shouldAddStubsEagerly = !... shouldAddStubsLazily ? https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:102: } else if (firstChar === 43) { // 43 is aka "+". is aka -> "is" or "aka". https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:235: function addStubs(descriptor, array, name, isStatic, functions) { Since you seem to know what these arguments are, please add comments. (In particular what "functions" is. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:245: if (isStatic) functions.push(name); Is this necessary in this CL? https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/codegen_helpers.dart (right): https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen_helpers.dart:394: // C.prototype.method.call(instance) indent by 4. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen_helpers.dart:397: // get of prototype might be evaluted before instance is created if we evaluated https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen_helpers.dart:399: // after first access if we use lazy initialization. Also add to the comment, how you work around it? (basically, from what I can see, you skip the first argument, the receiver). For example: In this case, we therefore don't allow the receiver (the first argument) to be generated at use site, and only analyze all other arguments. https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_names.dart (right): https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:50: /// Implements a mapping from mangled names to their refletive counterparts. reflective https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:54: var _jsMangledNames; Document what type it is here (and not in the dartdoc of the class). https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:64: return filter ? null : result; You should type it, so that the inferrer realizes that this must be a string. https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:79: int setterPrefixLength = getterPrefix.length; getterPrefixLength ? (If not explain) https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:94: /// object changes its lenght. This condition is sufficient as the name mapping length https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:99: final _jsMangledNames; Document what (type) this field is. https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:117: result[reflectiveName] = key; ditto: make sure that the type is known.
https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right): https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:401: }''', { 'deferredAction': js.string(namer.deferredAction), On 2015/03/11 13:59:33, floitsch wrote: > potentially create the string here? > (although maybe not worth it). > > that is: > body += #deferredActionCall > > > 'deferredActionCall': js.string("this.${namer.deferredAction}()") I would prefer to leave the fact that it is a call in the js template. I already don't like that I have to make it a string. We should have a way to expand a jsAst argument into a string. Maybe ##? https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart (right): https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:59: var prototype = prototype || this; On 2015/03/11 13:59:33, floitsch wrote: > document this. Done. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:94: var deferWork = supportsDirectProtoAccess && cls != #objectClassName; On 2015/03/11 13:59:33, floitsch wrote: > shouldDeferWork ? > shouldAddStubsEagerly = !... > shouldAddStubsLazily ? Done. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:102: } else if (firstChar === 43) { // 43 is aka "+". On 2015/03/11 13:59:33, floitsch wrote: > is aka -> "is" or "aka". Argh, missed that one. Thanks, done. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:235: function addStubs(descriptor, array, name, isStatic, functions) { On 2015/03/11 13:59:33, floitsch wrote: > Since you seem to know what these arguments are, please add comments. > (In particular what "functions" is. Done. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:245: if (isStatic) functions.push(name); On 2015/03/11 13:59:33, floitsch wrote: > Is this necessary in this CL? Great catch. That was a left-over from an experiment. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/codegen_helpers.dart (right): https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen_helpers.dart:394: // C.prototype.method.call(instance) On 2015/03/11 13:59:34, floitsch wrote: > indent by 4. Done. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen_helpers.dart:397: // get of prototype might be evaluted before instance is created if we On 2015/03/11 13:59:34, floitsch wrote: > evaluated Done. https://codereview.chromium.org/974803002/diff/100001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen_helpers.dart:399: // after first access if we use lazy initialization. On 2015/03/11 13:59:34, floitsch wrote: > Also add to the comment, how you work around it? > (basically, from what I can see, you skip the first argument, the receiver). > > For example: > > In this case, we therefore don't allow the receiver (the first argument) to be > generated at use site, and only analyze all other arguments. Done. https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_names.dart (right): https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:50: /// Implements a mapping from mangled names to their refletive counterparts. On 2015/03/11 13:59:34, floitsch wrote: > reflective Done. https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:54: var _jsMangledNames; On 2015/03/11 13:59:34, floitsch wrote: > Document what type it is here (and not in the dartdoc of the class). Done. https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:64: return filter ? null : result; On 2015/03/11 13:59:34, floitsch wrote: > You should type it, so that the inferrer realizes that this must be a string. Done. https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:79: int setterPrefixLength = getterPrefix.length; On 2015/03/11 13:59:34, floitsch wrote: > getterPrefixLength ? > (If not explain) Done. https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:94: /// object changes its lenght. This condition is sufficient as the name mapping On 2015/03/11 13:59:34, floitsch wrote: > length Done. https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:99: final _jsMangledNames; On 2015/03/11 13:59:34, floitsch wrote: > Document what (type) this field is. Done. https://codereview.chromium.org/974803002/diff/100001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_names.dart:117: result[reflectiveName] = key; On 2015/03/11 13:59:34, floitsch wrote: > ditto: make sure that the type is known. Done.
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 44458 (presubmit successful). |
