Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
707793002:
Use field names that are unique only wrt. the inheritance chain in minified mode. (Closed)
|
Created:
6 years, 1 month ago by herhut Modified:
6 years ago CC:
reviews_dartlang.org, floitsch Visibility:
Public. |
DescriptionUse field names that are unique only wrt. the inheritance chain in minified mode.
R=floitsch@google.com, sra@google.com
BUG= http://dartbug.com/21510
Committed: https://code.google.com/p/dart/source/detail?r=41986
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Reworked naming scheme and added test. #
Total comments: 26
Patch Set 3 : Comments #
Messages
Total messages: 9 (3 generated)
Patchset #1 (id:1) has been deleted
floitsch@google.com changed reviewers: + floitsch@google.com
LGTM with comments.
* Tests. At least add:
- a class that needs 2-character names.
- a mixin with potentially conflicting names ('a', 'b', ...)
* No static state (or a good plan on how to deal with it).
https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/minify_namer.dart (right):
https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:237: return
super.instanceFieldPropertyName(element);
When can this happen? (add comment).
How do you ensure that the super-proposed names don't conflict with your shorter
names?
From what I can see the main-reason this happens is, when there is a mixin.
However we still use shorter names for subclasses. Can't they conflict?
https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:252: * Obviously, this only
works if no fields are added to a parent node after its
New line before.
https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:261: static Map<Entity,
_FieldNamingScope> _scopes =
When is this map cleared?
This looks like something that could blow up in --batch and incremental
compilation.
https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:295:
_FieldNamingScope.rootScope(this.container) :
: in next line.
https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:296: superScope = null,
indent by 4.
https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:303: _valueIsUnused(String
name) {
_isValueUnused
Patchset #2 (id:40001) has been deleted
PTAL https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_... File pkg/compiler/lib/src/js_backend/minify_namer.dart (right): https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:237: return super.instanceFieldPropertyName(element); On 2014/11/06 14:05:41, floitsch wrote: > When can this happen? (add comment). > How do you ensure that the super-proposed names don't conflict with your shorter > names? > > From what I can see the main-reason this happens is, when there is a mixin. > However we still use shorter names for subclasses. Can't they conflict? I have reworked this and included support for naming Mixin fields directly. https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:252: * Obviously, this only works if no fields are added to a parent node after its On 2014/11/06 14:05:41, floitsch wrote: > New line before. Done. https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:261: static Map<Entity, _FieldNamingScope> _scopes = On 2014/11/06 14:05:41, floitsch wrote: > When is this map cleared? > > This looks like something that could blow up in --batch and incremental > compilation. I have factored the state out into a field of the namer. https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:295: _FieldNamingScope.rootScope(this.container) : On 2014/11/06 14:05:41, floitsch wrote: > : in next line. Done. https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:296: superScope = null, On 2014/11/06 14:05:41, floitsch wrote: > indent by 4. Done. https://codereview.chromium.org/707793002/diff/20001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:303: _valueIsUnused(String name) { On 2014/11/06 14:05:41, floitsch wrote: > _isValueUnused Done.
LGTM. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... File pkg/compiler/lib/src/js_backend/minify_namer.dart (right): https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:274: // Try reuse short reserved names (which are safe for fields). Otherwise Start by saying that the namer doesn't use reserved names, but some of them are short and we thus want to shortcut the namer for them. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:307: int _count; Document _count. Apparently it serves as both maximum and local count. That's confusing. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:309: // The maxium count along the superclass chain. maximum https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:312: // The local count. This is separate to be able to override it in subclasses. What's a "local" count? Why is it the same as the maxCount. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:318: _FieldNamingScope computeFieldNames() { new line before nested function. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:320: // For a minin class, we have to generate globally unique names "mixin". finish with a ".". Nit: I'm not sure how the comment helps here. It's already on the MixinFieldNamingScope. Here we just need to know that it needs to be treated specially (which is clear from the code). https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:324: ClassElement superClass = cls.superclass; apparently we generally use "superclass" and not "superClass". https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:328: // Mixin applications use the global scope. Same nit for comment: it's not clear how the "new _MixinFieldNamingScope" satisfies the "use the global scope". Rather move that comment to where it is relevant (where it probably already is). https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:330: result = new _MixinFieldNamingScope.mixedin(cls, superScope, mixedIn ? https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:335: } else { I think it's easier to read if this is the default case. Then the actual condition is close to the 'else'. if (superClass == null) { result = new _FieldNamingScope.rootScope(cls, registry); } else { superClass = cls.superclass; ... } Maybe even avoid the 'superClass' local. if (world.isUsedAsMixin(cls)) { result = new _MixinFieldNamingScope.mixin(cls, registry); } else if (cls.superclass == null) { result = new _FieldNamingScope.rootScope(cls, registry); } else { _FieldNamingScope superScope = new _FieldNamingScope.forClass(cls.superclass, world, registry); if (cls.isMixinApplication) { result = new _MixinFieldNamingScope.mixedin(cls, superScope, registry); } else { result = new _FieldNamingScope.inherit(cls, superScope, registry); } } Finally: couldn't the mixedIn and inherit constructors just construct the super-scope themselves? https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:344: return registry.scopes.putIfAbsent(cls, computeFieldNames); I prefer the more common pattern: _FieldNamingScope cached = registry.scopes[cls]; if (cached != null) return cached; <code that was inside computeFieldNames> registry[cls] = result; return result; https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:360: _isValueUnused(String name) { comment what values we are talking about. I would have thought that this is "isNameUnused". https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:388: * mixins and add a `$` at the end to ensure they are unqiue across all field how does the "$" guarantee that it's unique across all field names? add assert here and in other locations: assert(isReserved(name)); // or isReservedForMixin(name); and in the other uses: assert(!isReserved(name)); // or !isReservedForMixin(name);
https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... File pkg/compiler/lib/src/js_backend/minify_namer.dart (right): https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:274: // Try reuse short reserved names (which are safe for fields). Otherwise On 2014/11/19 13:04:26, floitsch wrote: > Start by saying that the namer doesn't use reserved names, but some of them are > short and we thus want to shortcut the namer for them. Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:307: int _count; On 2014/11/19 13:04:26, floitsch wrote: > Document _count. > Apparently it serves as both maximum and local count. That's confusing. Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:309: // The maxium count along the superclass chain. On 2014/11/19 13:04:26, floitsch wrote: > maximum Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:312: // The local count. This is separate to be able to override it in subclasses. On 2014/11/19 13:04:26, floitsch wrote: > What's a "local" count? > Why is it the same as the maxCount. Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:318: _FieldNamingScope computeFieldNames() { On 2014/11/19 13:04:25, floitsch wrote: > new line before nested function. Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:320: // For a minin class, we have to generate globally unique names On 2014/11/19 13:04:26, floitsch wrote: > "mixin". > finish with a ".". > > Nit: > I'm not sure how the comment helps here. It's already on the > MixinFieldNamingScope. Here we just need to know that it needs to be treated > specially (which is clear from the code). Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:324: ClassElement superClass = cls.superclass; On 2014/11/19 13:04:26, floitsch wrote: > apparently we generally use "superclass" and not "superClass". Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:328: // Mixin applications use the global scope. On 2014/11/19 13:04:26, floitsch wrote: > Same nit for comment: it's not clear how the "new _MixinFieldNamingScope" > satisfies the "use the global scope". Rather move that comment to where it is > relevant (where it probably already is). Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:330: result = new _MixinFieldNamingScope.mixedin(cls, superScope, On 2014/11/19 13:04:26, floitsch wrote: > mixedIn ? Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:335: } else { On 2014/11/19 13:04:26, floitsch wrote: > I think it's easier to read if this is the default case. Then the actual > condition is close to the 'else'. > > if (superClass == null) { > result = new _FieldNamingScope.rootScope(cls, registry); > } else { > superClass = cls.superclass; > ... > } > > Maybe even avoid the 'superClass' local. > > if (world.isUsedAsMixin(cls)) { > result = new _MixinFieldNamingScope.mixin(cls, registry); > } else if (cls.superclass == null) { > result = new _FieldNamingScope.rootScope(cls, registry); > } else { > _FieldNamingScope superScope = > new _FieldNamingScope.forClass(cls.superclass, world, registry); > if (cls.isMixinApplication) { > result = new _MixinFieldNamingScope.mixedin(cls, superScope, registry); > } else { > result = new _FieldNamingScope.inherit(cls, superScope, registry); > } > } I have reordered them. > Finally: couldn't the mixedIn and inherit constructors just construct the > super-scope themselves? Then I would also have to pass [world] to them and I prefer to keep the dependency to world in the factory. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:344: return registry.scopes.putIfAbsent(cls, computeFieldNames); On 2014/11/19 13:04:26, floitsch wrote: > I prefer the more common pattern: > > _FieldNamingScope cached = registry.scopes[cls]; > if (cached != null) return cached; > > <code that was inside computeFieldNames> > registry[cls] = result; > return result; Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:360: _isValueUnused(String name) { On 2014/11/19 13:04:26, floitsch wrote: > comment what values we are talking about. > I would have thought that this is "isNameUnused". Done. https://codereview.chromium.org/707793002/diff/60001/pkg/compiler/lib/src/js_... pkg/compiler/lib/src/js_backend/minify_namer.dart:388: * mixins and add a `$` at the end to ensure they are unqiue across all field On 2014/11/19 13:04:26, floitsch wrote: > how does the "$" guarantee that it's unique across all field names? > add assert here and in other locations: I have expanded the comment to explain this. > assert(isReserved(name)); // or isReservedForMixin(name); > > and in the other uses: > assert(!isReserved(name)); // or !isReservedForMixin(name); The uniqueness is checked when adding a field (the add method does so by asserting on isNameUnused).
Message was sent while issue was closed.
Committed patchset #3 (id:80001) manually as 41986 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
