Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
841993003:
dart2js: Only emit constructors of the native class interceptors that are used. (Closed)
|
Created:
5 years, 11 months ago by zarah Modified:
5 years, 11 months ago CC:
reviews_dartlang.org Target Ref:
refs/remotes/git-svn Visibility:
Public. |
Descriptiondart2js: Only emit constructors of the native class interceptors that are used.
This change only affects CSP mode: when compiling in CSP mode we were emitting constructors and getters/setters of native classes that were potentially not needed.
R=floitsch@google.com
Committed: https://code.google.com/p/dart/source/detail?r=42944
Reverted: https://code.google.com/p/dart/source/detail?r=42950
Committed: https://code.google.com/p/dart/source/detail?r=43001
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added documentation. #
Total comments: 8
Patch Set 3 : Addressed comments. #
Total comments: 6
Patch Set 4 : Updated comment. #Patch Set 5 : Rebase after revert #Patch Set 6 : Split generation of checked setters and getters/setters for CSP. #Patch Set 7 : Incorporating changes from Issue 854133003. Adds return in emitClassConstructor plus some renames. #
Total comments: 12
Patch Set 8 : Addressed comments. #
Messages
Total messages: 23 (5 generated)
zarah@google.com changed reviewers: + floitsch@google.com, sra@google.com
The patch seems to do the right thing, but documentation, verification is missing. I'm ok with splitting the native class generation (as you do), since we will replace it with the model. However, it needs to be well documented, and ideally have some asserts to convince readers that this is not introducing bugs. https://codereview.chromium.org/841993003/diff/1/pkg/compiler/lib/src/js_back... File pkg/compiler/lib/src/js_backend/native_emitter.dart (right): https://codereview.chromium.org/841993003/diff/1/pkg/compiler/lib/src/js_back... pkg/compiler/lib/src/js_backend/native_emitter.dart:240: emitterTask.oldEmitter.classEmitter.emitClassConstructor( Requires lots of comments, and verification. As far as I can see (please confirm and document): - we call generateNativeClass first, so that we have a builder that can tell us, if the class `isTrivial`. (Sidenote: it's interesting that we have sets/maps everywhere, but that we store the `isTrivial` in the class-builder, even though the class builder is most often used for non-native classes.) The `isTrivial` flag is dependent on the number of members, and it doesn't look like it's easy to get the flag without running through all members and generating them... (Hopefully we can fix that when building the model). - Apparently emitting the class constructor or the getters/setters adds code outside the builder. From what I can see this is only happening in CSP mode. (If it happens only in CSP mode you have to add a comment in the CL description, otherwise reviewers are in the dark). This is a problem: if we figure out that the class is not needed we still have those entries. It looks like we don't need `emitClassGettersSetters` to identify if a native class is needed or not, because getters/setters are only emitted for fields, and in that case the `isTrivial` is already false. At least I was not able to write a counterexample where moving the emitClassGettersSetters would have changed the outcome of `neededClasses`. The `emitClassConstructor` seems to be safe to move, too. As far as I can tell its only purpose is to generate the constructors in CSP mode. As such it shouldn't have any influence on the `neededClasses` either.
The description "dart2js: Only emit constructors of the native class interceptors that are used" indicates that there are cases where native class constructors are emitted but unused. Can you explain when that happens? If I patch this CL and compile swarm, I see no changes. What do I need to do to see a change?
On 2015/01/13 19:16:06, sra1 wrote: > The description "dart2js: Only emit constructors of the native class > interceptors that are used" > indicates that there are cases where native class constructors are emitted but > unused. > Can you explain when that happens? > If I patch this CL and compile swarm, I see no changes. > What do I need to do to see a change? This only affects CSP mode. So add --csp to the command line options you use when compiling Swarm.
PTAL, thanks. https://codereview.chromium.org/841993003/diff/1/pkg/compiler/lib/src/js_back... File pkg/compiler/lib/src/js_backend/native_emitter.dart (right): https://codereview.chromium.org/841993003/diff/1/pkg/compiler/lib/src/js_back... pkg/compiler/lib/src/js_backend/native_emitter.dart:240: emitterTask.oldEmitter.classEmitter.emitClassConstructor( On 2015/01/13 18:40:57, floitsch wrote: > Requires lots of comments, and verification. > > As far as I can see (please confirm and document): > - we call generateNativeClass first, so that we have a builder that can tell us, > if the class `isTrivial`. (Sidenote: it's interesting that we have sets/maps > everywhere, but that we store the `isTrivial` in the class-builder, even though > the class builder is most often used for non-native classes.) > The `isTrivial` flag is dependent on the number of members, and it doesn't look > like it's easy to get the flag without running through all members and > generating them... (Hopefully we can fix that when building the model). > > - Apparently emitting the class constructor or the getters/setters adds code > outside the builder. From what I can see this is only happening in CSP mode. (If > it happens only in CSP mode you have to add a comment in the CL description, > otherwise reviewers are in the dark). > > This is a problem: if we figure out that the class is not needed we still have > those entries. > > > It looks like we don't need `emitClassGettersSetters` to identify if a native > class is needed or not, because getters/setters are only emitted for fields, and > in that case the `isTrivial` is already false. At least I was not able to write > a counterexample where moving the emitClassGettersSetters would have changed the > outcome of `neededClasses`. > > The `emitClassConstructor` seems to be safe to move, too. As far as I can tell > its only purpose is to generate the constructors in CSP mode. As such it > shouldn't have any influence on the `neededClasses` either. I agree with your observations. Updated the CL description and added comments.
ahe@google.com changed reviewers: + ahe@google.com
https://codereview.chromium.org/841993003/diff/20001/pkg/compiler/lib/src/js_... File pkg/compiler/lib/src/js_backend/native_emitter.dart (right): https://codereview.chromium.org/841993003/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/native_emitter.dart:241: // Having the calls here (instead of in the classBuilder) is safe: I'm not sure this comment adds value long term. When reading this code, why would you think that this code should be in a different place? I think this comment should be sufficient: /* In CSP mode [emitClassConstructor] and [emitClassGettersSetters] have a side-effect on "precompiled" functions in [OldEmitter]. For this reason, it is important that we don't call these methods before we are certain that a class is needed. */ https://codereview.chromium.org/841993003/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/native_emitter.dart:243: // [emitClassConstructor] only affects the generation of constructors Shouldn't this be part of emitClassConstructor's documentation comment? https://codereview.chromium.org/841993003/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/native_emitter.dart:246: // [emitClassGettersSetters] does not affect whether or not a class is Ditto.
LGTM once Peter's comments are addressed.
Small change of mind. (But still LGTM once the comments have been addressed). https://chromiumcodereview.appspot.com/841993003/diff/20001/pkg/compiler/lib/... File pkg/compiler/lib/src/js_backend/native_emitter.dart (right): https://chromiumcodereview.appspot.com/841993003/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_backend/native_emitter.dart:243: // [emitClassConstructor] only affects the generation of constructors On 2015/01/15 08:49:51, ahe wrote: > Shouldn't this be part of emitClassConstructor's documentation comment? Probably both. https://chromiumcodereview.appspot.com/841993003/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_backend/native_emitter.dart:246: // [emitClassGettersSetters] does not affect whether or not a class is On 2015/01/15 08:49:51, ahe wrote: > Ditto. No. this comment should be here. It explains why we can delay the call to here, and don't need to run it below. However it could/should also be in the documentation of the function.
https://chromiumcodereview.appspot.com/841993003/diff/20001/pkg/compiler/lib/... File pkg/compiler/lib/src/js_backend/native_emitter.dart (right): https://chromiumcodereview.appspot.com/841993003/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_backend/native_emitter.dart:241: // Having the calls here (instead of in the classBuilder) is safe: On 2015/01/15 08:49:51, ahe wrote: > I'm not sure this comment adds value long term. When reading this code, why > would you think that this code should be in a different place? > > I think this comment should be sufficient: > > /* > In CSP mode [emitClassConstructor] and [emitClassGettersSetters] have a > side-effect on "precompiled" functions in [OldEmitter]. For this reason, it is > important that we don't call these methods before we are certain that a class is > needed. > */ Done. https://chromiumcodereview.appspot.com/841993003/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_backend/native_emitter.dart:243: // [emitClassConstructor] only affects the generation of constructors On 2015/01/15 08:49:51, ahe wrote: > Shouldn't this be part of emitClassConstructor's documentation comment? Added documentation of the function as well. https://chromiumcodereview.appspot.com/841993003/diff/20001/pkg/compiler/lib/... pkg/compiler/lib/src/js_backend/native_emitter.dart:246: // [emitClassGettersSetters] does not affect whether or not a class is On 2015/01/15 14:37:20, floitsch wrote: > On 2015/01/15 08:49:51, ahe wrote: > > Ditto. > > No. this comment should be here. > It explains why we can delay the call to here, and don't need to run it below. > > However it could/should also be in the documentation of the function. Added documentation of the function as well.
LGTM. https://chromiumcodereview.appspot.com/841993003/diff/40001/pkg/compiler/lib/... File pkg/compiler/lib/src/js_backend/native_emitter.dart (right): https://chromiumcodereview.appspot.com/841993003/diff/40001/pkg/compiler/lib/... pkg/compiler/lib/src/js_backend/native_emitter.dart:236: // In CSP mode [emitClassConstructor] and [emitClassGettersSetters] have nit: newline before. https://chromiumcodereview.appspot.com/841993003/diff/40001/pkg/compiler/lib/... pkg/compiler/lib/src/js_backend/native_emitter.dart:245: // [emitClassGettersSetters] does not affect whether or not a class is nit: newline before. https://chromiumcodereview.appspot.com/841993003/diff/40001/pkg/compiler/lib/... File pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart (right): https://chromiumcodereview.appspot.com/841993003/diff/40001/pkg/compiler/lib/... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:225: * [emitClassGettersSetters] does not affect whether or not a class is needed That comment is out of context. It should say something like: If necessary, emits getters and setters for fields. They can be needed in CSP mode, or in checked mode, when types need to be verified.
https://codereview.chromium.org/841993003/diff/40001/pkg/compiler/lib/src/js_... File pkg/compiler/lib/src/js_backend/native_emitter.dart (right): https://codereview.chromium.org/841993003/diff/40001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/native_emitter.dart:236: // In CSP mode [emitClassConstructor] and [emitClassGettersSetters] have On 2015/01/15 15:14:40, floitsch wrote: > nit: newline before. Done. https://codereview.chromium.org/841993003/diff/40001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/native_emitter.dart:245: // [emitClassGettersSetters] does not affect whether or not a class is On 2015/01/15 15:14:40, floitsch wrote: > nit: newline before. Done. https://codereview.chromium.org/841993003/diff/40001/pkg/compiler/lib/src/js_... File pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart (right): https://codereview.chromium.org/841993003/diff/40001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:225: * [emitClassGettersSetters] does not affect whether or not a class is needed On 2015/01/15 15:14:40, floitsch wrote: > That comment is out of context. > > It should say something like: > If necessary, emits getters and setters for fields. > > They can be needed in CSP mode, or in checked mode, when types need to be > verified. You are right. Comment updated.
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 42944 (presubmit successful).
Message was sent while issue was closed.
Patchset #5 (id:80001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
PTAL
LGTM with comments. https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart (right): https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:46: emitClassGettersSettersForCSP(classElement, builder, onlyForRti: onlyForRti); Long line. https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:73: if (!compiler.useContentSecurityPolicy Are you sure this is correct? I think we still need the constructorAst, even when we don't need the fields (as in onlyForRti). https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:249: void emitClassGettersSettersForCSP(ClassElement classElement, Should be consistent. Either rename emitPrecompiledConstructors to emitConstructorsForCSP, or rename here. https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:534: void generateGetterForCSP(Element member, String fieldName, String accessorName, long line. https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:534: void generateGetterForCSP(Element member, String fieldName, String accessorName, I would rename to "emitGetterForCSP". I use "generate" when something is returned and it doesn't have a side-effect. "Emit", when something is emitted (thus having a side-effect). The line is blurred, when the generated thing is stored in the builder, but I still prefer `emit` there. https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:553: void generateSetterForCSP(Element member, String fieldName, String accessorName, long line.
https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart (right): https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:46: emitClassGettersSettersForCSP(classElement, builder, onlyForRti: onlyForRti); On 2015/01/19 13:40:31, floitsch wrote: > Long line. Done. https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:73: if (!compiler.useContentSecurityPolicy On 2015/01/19 13:40:32, floitsch wrote: > Are you sure this is correct? > > I think we still need the constructorAst, even when we don't need the fields (as > in onlyForRti). Done. https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:249: void emitClassGettersSettersForCSP(ClassElement classElement, On 2015/01/19 13:40:31, floitsch wrote: > Should be consistent. > Either rename emitPrecompiledConstructors to emitConstructorsForCSP, or rename > here. Done. https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:534: void generateGetterForCSP(Element member, String fieldName, String accessorName, On 2015/01/19 13:40:31, floitsch wrote: > long line. Done. https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:534: void generateGetterForCSP(Element member, String fieldName, String accessorName, On 2015/01/19 13:40:31, floitsch wrote: > I would rename to "emitGetterForCSP". > > I use "generate" when something is returned and it doesn't have a side-effect. > "Emit", when something is emitted (thus having a side-effect). > The line is blurred, when the generated thing is stored in the builder, but I > still prefer `emit` there. Done. https://codereview.chromium.org/841993003/diff/180001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:553: void generateSetterForCSP(Element member, String fieldName, String accessorName, On 2015/01/19 13:40:32, floitsch wrote: > long line. Done.
Message was sent while issue was closed.
Committed patchset #8 (id:200001) manually as 43001 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
