[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[scottb]

LGTM w/nits, no need to re-review.


http://gwt-code-reviews.appspot.com/1441803/diff/3010/dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java
File dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java
(right):

http://gwt-code-reviews.appspot.com/1441803/diff/3010/dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java#newcode45
dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java:45:
public CachedCompilationUnit(CachedCompilationUnit unit, long
lastModified,
Javadoc.

http://gwt-code-reviews.appspot.com/1441803/diff/3010/dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java#newcode66
dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java:66: }
You should just be able to just assign the field, the conversion would
have happened already in the original unit.

http://gwt-code-reviews.appspot.com/1441803/diff/3010/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1441803/diff/3010/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode3
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:3: *
whitespace

http://gwt-code-reviews.appspot.com/1441803/diff/3010/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode404
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:404:
unitCache.remove(cachedUnit);
It looks like adding the new unit is supposed to bump out the old one.

http://gwt-code-reviews.appspot.com/1441803/diff/3010/dev/core/src/com/google/gwt/dev/util/DiskCacheToken.java
File dev/core/src/com/google/gwt/dev/util/DiskCacheToken.java (right):

http://gwt-code-reviews.appspot.com/1441803/diff/3010/dev/core/src/com/google/gwt/dev/util/DiskCacheToken.java#newcode52
dev/core/src/com/google/gwt/dev/util/DiskCacheToken.java:52: }
I think this can be reverted now

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

Crap, the reformatter went nuts, I'll post when its ready.

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

Patch with just reformatting is up at

http://gwt-code-reviews.appspot.com/1444802/

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

And yes, this patch is now ready for review.


http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[scottb]

Mostly LGTM w/ comments.


http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode407
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:407:
}
How important is this part, performance wise?  Seems like it would
simplify things a bit to leave CCU immutable and not update the
location/modified.

If it is important, instead of two different setters, a single
update(location, lastModified) would be nicer semantically.

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java
File dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java (right):

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java#newcode215
dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java:215: public
abstract CachedCompilationUnit asCachedCompilationUnit();
With this, should make writeReplace() final and concrete, methinks.

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode407
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:407:
}
On 2011/05/11 22:28:44, scottb wrote:

How important is this part, performance wise?  Seems like it would

simplify

things a bit to leave CCU immutable and not update the

location/modified.


If it is important, instead of two different setters, a single

update(location,

lastModified) would be nicer semantically.


Calculating the contentId for the builder requires reading the source
and performing an MD5 on the contents.  Let's say, that someone upgrades
or re-compiles gwt-user.jar with a small patch.  This means that each
time the compilation builder starts, the timestamp will be from old jar,
but the actual contents will be the same.  This means we open the
resource passed to 'builder' and scan through it to compute the content
ID, then we throw away the builder and use the bytecode from the cached
unit (which is actually a different set of bytes, although they should
be identical).

I didn't measure the exact impact, but my intuition is that an extra
scan of all the source in gwt-user is more expensive than not scanning
at all.

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java
File dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java (right):

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java#newcode215
dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java:215: public
abstract CachedCompilationUnit asCachedCompilationUnit();
On 2011/05/11 22:28:44, scottb wrote:

With this, should make writeReplace() final and concrete, methinks.


Done.

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[scottb]

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode407
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:407:
}
That makes total sense.  Then I would suggest either the update() method
as described, or perhaps better yet, a new quick copy constructor that
takes additional lastModifed and location and otherwise copies all
values from the old guy, just to give the new unit a new identity and
keep the immutability.  Otherwise, someone who already has a reference
to the old unit could observe that unit to change.  Makes debugging
trickier too.

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode407
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:407:
}
On 2011/05/11 23:42:11, scottb wrote:

That makes total sense.  Then I would suggest either the update()

method as

described, or perhaps better yet, a new quick copy constructor that

takes

additional lastModifed and location and otherwise copies all values

from the old

guy, just to give the new unit a new identity and keep the

immutability.

Otherwise, someone who already has a reference to the old unit could

observe

that unit to change.  Makes debugging trickier too.


I believe that's essentially what I did in patch set 2 - the old unit is
copied with asCachedCompilationUnit() and then I update the values in
the copy.  Maybe I should rename asCachedCompilationUnit() to something
else to make that more clear?

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[scottb]

Mostly LG.


http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode407
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:407:
}
If I'm reading this correctly,
CachedCompilationUnit.asCachedCompilationUnit() returns the original
object, which you then modify.  I'm explicitly suggesting:

new CachedCompilationUnit(CachedCompilationUnit original, String
updatedResourceLocation, long updatedLastModified)

http://gwt-code-reviews.appspot.com/1441803/diff/4003/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode3
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:3: *
FYI: your EOL-fu is unhappy.

http://gwt-code-reviews.appspot.com/1441803/diff/4003/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java
File dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java
(right):

http://gwt-code-reviews.appspot.com/1441803/diff/4003/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode56
dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:56:
return new CachedCompilationUnit(this, sourceToken, astToken);
Unfortunate, but understandable.

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

http://gwt-code-reviews.appspot.com/1441803/diff/4003/dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java
File dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java
(right):

http://gwt-code-reviews.appspot.com/1441803/diff/4003/dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java#newcode87
dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java:87:
return this;
this needs to return a copy. all other implementations of
asCachedCompilationUnit() already return a copy.

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[Scott Blum]

On Wed, May 11, 2011 at 10:57 PM, zun...@google.com wrote:

 this needs to return a copy. all other implementations of
 asCachedCompilationUnit() already return a copy.


That would make CachedCompilationUnit.writeReplace() be weird tho, right?

-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Rescues cached entries from jar files that are the same, save for the timestamp. (issue1441803)

[zundel]

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1441803/diff/4001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode407
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:407:
}
On 2011/05/12 02:39:19, scottb wrote:

If I'm reading this correctly,

CachedCompilationUnit.asCachedCompilationUnit()

returns the original object, which you then modify.  I'm explicitly

suggesting:


new CachedCompilationUnit(CachedCompilationUnit original, String
updatedResourceLocation, long updatedLastModified)


OK, I see your pointnow. In this part of the code 'cachedUnit' is not
necessarily an instance of CachedCompilationUnit so I had to add all
this infrastructure to be able to get one.  I was confused because in
most places, asCachedCompilationUnit() does make a copy, but I forgot
about CachedCompilationUnit itself.

http://gwt-code-reviews.appspot.com/1441803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors