http://gwt-code-reviews.appspot.com/1236801/diff/39001/40001 File dev/core/src/com/google/gwt/core/ext/typeinfo/JRealClassType.java (right):
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40001#newcode28 dev/core/src/com/google/gwt/core/ext/typeinfo/JRealClassType.java:28: void addByteCode(byte[] byteCode); On 2011/01/24 19:14:07, tobyr wrote:
Why is this method in the public interface? Can we just remove it and
leave the
implementation in typemodel.JRealClassType?
Well, I thought about doing that, but then we'd lose the benefits of the interface layer. The generator calling getTypeStrongHash shouldn't need to know the specific implementation for JRealClassType (and of type oracle). The generators refer to the interface versions of all the type oracle and class type apis, and so this was in keeping with that. Once we get the changes into TypeOracleMediator to be able to more specifically check type structure/version, we can add a capability interface (as Bob suggests), e.g. HasTypeStructureSignature. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40003 File dev/core/src/com/google/gwt/dev/javac/rebind/CachedClientDataMap.java (right): http://gwt-code-reviews.appspot.com/1236801/diff/39001/40003#newcode39 dev/core/src/com/google/gwt/dev/javac/rebind/CachedClientDataMap.java:39: // this will throw a ClassCastException if value is not Serializable On 2011/01/24 19:14:07, tobyr wrote:
Comment is pointless as is. Document with javadoc instead?
Agreed, I've removed the comment http://gwt-code-reviews.appspot.com/1236801/diff/39001/40008 File dev/core/src/com/google/gwt/dev/javac/typemodel/JRealClassType.java (right): http://gwt-code-reviews.appspot.com/1236801/diff/39001/40008#newcode290 dev/core/src/com/google/gwt/dev/javac/typemodel/JRealClassType.java:290: lazyTypeStrongHash = Util.computeStrongName(byteCode); On 2011/01/24 19:14:07, tobyr wrote:
This isn't exactly what we want either, but it's better than assuming
always
stale or taking a long time to compute a hash. Can you add a comment
here that
explains that ideally we want to identify staleness based on a type's
structure,
not including its code, but we don't have a quick way to compute a
hash for type
structure yet?
I've added a TODO to revisit implementing an efficient hash of the type structure. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40010 File user/src/com/google/gwt/resources/ext/ClientBundleRequirements.java (right): http://gwt-code-reviews.appspot.com/1236801/diff/39001/40010#newcode50 user/src/com/google/gwt/resources/ext/ClientBundleRequirements.java:50: * whether a configuration property by the same name exists. On 2011/01/24 19:33:31, bobv wrote:
This sounds sloppy.
Well, this was current behavior (I didn't add). Should it be removed? I added the comment in the javadoc, to make it clear, as I noticed the discrepancy in the description. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40011 File user/src/com/google/gwt/resources/ext/ResourceContext.java (right): http://gwt-code-reviews.appspot.com/1236801/diff/39001/40011#newcode143 user/src/com/google/gwt/resources/ext/ResourceContext.java:143: ClientBundleRequirements getRequirements(); On 2011/01/24 19:33:31, bobv wrote:
In order to compute the simple name of the implementing ClientBundle
type, it is
necessary to know all of the permutation properties that affect the
generated
code. If the requirements object can be accessed at arbitrary point
in time, it
would be possible to add a property requirement after the GeneratorContext.tryCreate() call has been made.
Yeah, I suppose so. Essentially now the Requirements class has 2 uses (whereas before it only had one, the simple file name). Now I'm also using it for tracking selection/configuration properties and type dependencies, for the purpose of checking cacheability. FWIW, I don't see anywhere there where I've added calls to "addPermutationAxis()" after the tryCreate call (but there are now subsequent calls to requirements.AddConfigurationProperty() and requirements.AddTypeHierarchy()). I think there was the danger anyway prior to this change, that the Requirements object could have been updated after the call to get the simple name. I agree it's now easier for someone to do so since it's part of the context. But there is still no guard against a code path that might take an alternate code path based on a selection property after the simple name has been assigned. I suppose we can add a call to disable any further updates to any new permutationAxis entries. E.g. Requirements.disableFurtherPermutationAxisUpdates() (need a better name!). And then throw an exception if anyone tries to add a permutationAxis afterwards. Although that may be restrictive, in that it prevents selection properties being subsequently added to the set of dependencies. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40011#newcode153 user/src/com/google/gwt/resources/ext/ResourceContext.java:153: * interface can expect to call this method successfully. On 2011/01/24 19:33:31, bobv wrote:
The implementation does not enforce this.
Well, the implementation will only have attempted to find the resources for a CB if it's RG implements HFRD. So, that's the only way that this method can return with a non-null value. I'll change the wording to clarify (e.g. "Only RG's which implement HFRD can expect to retrieve non-null data using this method". http://gwt-code-reviews.appspot.com/1236801/diff/39001/40013 File user/src/com/google/gwt/resources/ext/SupportsGeneratorResultCaching.java (right): http://gwt-code-reviews.appspot.com/1236801/diff/39001/40013#newcode41 user/src/com/google/gwt/resources/ext/SupportsGeneratorResultCaching.java:41: public interface HasFindableResourceDependencies extends On 2011/01/24 19:14:07, tobyr wrote:
It's cute, but is there actually a benefit to nesting this?
Hmmmm, well, there's no reason particularly, and god knows I don't want to be cute. I'll move it out. That is if Bob doesn't convince me to get rid of it altogether. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40013#newcode41 user/src/com/google/gwt/resources/ext/SupportsGeneratorResultCaching.java:41: public interface HasFindableResourceDependencies extends On 2011/01/24 19:33:31, bobv wrote:
Why is this interface necessary when accumulation of source files
could just as
easily be added to the Requirements interface?
Well, in fact I did add type dependency tracking to the Requirements interface (addTypeHierarchy()). Oh, I guess you were referring to resource source files (and not source types)? This interface is just a marker so that the generator can know if a ResourceGenerator has resources which can be retrieved by calling RGU.findResources (so it's not related to source type dependencies at all). This allows the main CB generator to find all the resources in the task list up front, since they all need to be found in order to do an up front check for staleness. Some ResourceGenerators don't have findable resources (but do support generator result caching), such as GwtCreateResourceGenerator. So, this interface allows the ACBGenerator to distinguish between those it should find resources for, etc. I thought about adding the resource dependencies to the Requirements, but this would ultimately result in duplicating the process of calling RGU.findResources. This is because each individual RG would need to call findResources for their own processing (as in the current code), but then ACBGenerator would need to call findResources for each RG up front the next time around if it sees it in the Requirements object. Thus the inefficiency of calling findResources twice per resource. One way around it, I suppose, would be to add a caching layer in the ResourceContext and allow it to lazily call findResources, etc. for a given JMethod. That might make things a bit cleaner, I suppose. But this would also imply the need to track all the resources found in the generator's RebindCache. Seems like unnecessary tracking when all the information is available via the ClientBundle's method list. Thoughts? http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014 File user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java (right): http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode232 user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:232: for (JClassType superType : superTypes) { On 2011/01/24 19:14:07, tobyr wrote:
Why is this for loop necessary? Doesn't getFlattenedSupertypeHierarchy
already
do this?
yep...yep...Thanks for catching (seems a relic of an incomplete refactoring)... http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode249 user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:249: if (!(type instanceof JRealClassType)) { On 2011/01/24 19:14:07, tobyr wrote:
Under what situations does this occur?
Not sure, but all the api's via which we've retrieved types at this point in the code, return a JClassType (and not a JRealClassType), so it seems false to assume that we only will ever see a JRealClassType here (even though it appears that is currently always the case). http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode260 user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:260: On 2011/01/24 19:33:31, bobv wrote:
Extra whitespace, here and elsewhere.
Done. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode397 user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:397: CachedPropertyInformation cpi = new CachedPropertyInformation(logger, On 2011/01/24 19:33:31, bobv wrote:
Use a builder pattern for this type, since the number of things that
affect
dependencies is likely to increase in the future.
I'm not sure that's necessary here. I've got separate objects for tracking properties, and for tracking source type dependencies. So, I think the model is to add separate entries in the client data map (which itself needs to be generic, since it's not specific to this one generator type). http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode484 user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:484: JRealClassType sourceRealType = (JRealClassType) sourceType; On 2011/01/24 19:33:31, bobv wrote:
It would be more consistent with the rest of the API to cast to a
capability
interface such as HasTypeSignature here, rather than using
JRealClassType. Worth keeping in mind. As we are still in the process of overhauling the dev mode TypeOracleMediator, I'd like to hold off on defining a "HasStrongTypeHash" interface, until we are sure this will be the final version of the api. We are likely to replace this with a more useful getTypeStructureSignature() method. Thus, I've added "EXPERIMENTAL" language to the javadoc for the relevant methods in JRealClassType for now. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode510 user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:510: * Loop through each of our ResouceGenerator classes, and check those that On 2011/01/24 19:14:07, tobyr wrote:
s/that that/that
Done. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode532 user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:532: // use the jar file itself for the modified date test On 2011/01/24 19:14:07, tobyr wrote:
Document why
Done. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode820 user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:820: logger, resourceContext, method); On 2011/01/24 19:33:31, bobv wrote:
This is insufficient if the ResourceGenerator uses external resources
not
defined via an @Source annotation.
Perhaps we can discuss further. My feeling is that if there's an external resource for which we can't reliably check a lastModifiedDate(), then that RG should not implement the HasFindableResourceDependencies marker interface, in which case we won't be in this code path (as currently that's the only way we are going about checking cacheability). Also, for any resource that doesn't ultimately map to a "file://" protocol will be determined to be uncacheable, in this initial version. Perhaps in a subsequent version we can be a bit more sophisticated and try alternate ways to detect staleness for external resources (possibly by doing a quick hash on the file). http://gwt-code-reviews.appspot.com/1236801/diff/39001/40020 File user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java (right): http://gwt-code-reviews.appspot.com/1236801/diff/39001/40020#newcode43 user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java:43: public final class ExternalTextResourceGenerator extends On 2011/01/24 19:33:31, bobv wrote:
FYI: Unnur is also working on a change to ETRG that may affect its use
of
properties.
I'll consult with Unnur, thanks. http://gwt-code-reviews.appspot.com/1236801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
