Optimize clone operation

Optimize clone operation
Committed: https://code.google.com/p/dart/source/detail?r=4143

[danrubel, 2012-02-09 11:29:33]

This provides no value on 64 bit server but provides 20% - 30% speed improvement
on 32 bit client JVM. Thanks to Dan G for the idea.

[kasperl, 2012-02-09 12:31:42]

LGTM. The code is much, much simpler this way. Do we use Cloneable and
Object.clone in other places?

http://codereview.chromium.org/9370016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartScanner.java (right):

http://codereview.chromium.org/9370016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:24: public static
class Position implements Cloneable {
Does this need to be Cloneable?

http://codereview.chromium.org/9370016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:79: public static
class Location implements Cloneable {
Does this need to be Cloneable?

http://codereview.chromium.org/9370016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:406: private
static class TokenData implements Cloneable {
Does this need to be Cloneable?

[zundel, 2012-02-09 12:37:39]

I've looked at this code before, I don' think there is a problem, but I want to
take a closer look when I get into the office.

What stinks about this part of the code IIRC is that Location and Position may
not always need to be cloned, but we don't know that at this point.  These
objects are taking up a lot of memory when I look at JProfiler and contain more
information than we need (offset, length AND column, line #).  

BUT, not to worry, I believe the refactoring of the scanner/parser is addressing
this.

[zundel, 2012-02-09 15:44:06]

http://codereview.chromium.org/9370016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartScanner.java (right):

http://codereview.chromium.org/9370016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:406: private
static class TokenData implements Cloneable {
On 2012/02/09 12:31:42, kasperl wrote:
> Does this need to be Cloneable?

No, not if we take out the super.clone() reference.  But Effecitve Java (Item
10) says this is the wrong thing to do if you are implementing clone() in a
nonfinal class.

http://codereview.chromium.org/9370016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:412: protected
TokenData clone() {
We ditch clone and turn this into a copy constructor.

TokenData(TokenData orig) { ... }

On the one hand, we'll have to manually copy the fields.  On the other hand, it
would solve the performance problem without us having a clone() implementation
that violates good advice.

http://codereview.chromium.org/9370016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:413: TokenData
clone = new TokenData();
super.clone() is the right way to implement clone() acc'd to Josh Bloch which
has to do with maintaining the relationship:  x.clone().getClass() == 
x.getClass() if someone were to subclass TokenData


We could also turn these classes into final classes, which solves the issue, but
is a bit subtle, we should certainly document it.

[danrubel, 2012-02-10 08:19:20]

Comments addressed. PTAL.

https://chromiumcodereview.appspot.com/9370016/diff/1/compiler/java/com/googl...
File compiler/java/com/google/dart/compiler/parser/DartScanner.java (right):

https://chromiumcodereview.appspot.com/9370016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:24: public static
class Position implements Cloneable {
On 2012/02/09 12:31:42, kasperl wrote:
> Does this need to be Cloneable?

Good point. Removed.

https://chromiumcodereview.appspot.com/9370016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:79: public static
class Location implements Cloneable {
On 2012/02/09 12:31:42, kasperl wrote:
> Does this need to be Cloneable?

Done.

https://chromiumcodereview.appspot.com/9370016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:406: private
static class TokenData implements Cloneable {
On 2012/02/09 12:31:42, kasperl wrote:
> Does this need to be Cloneable?

Done.

https://chromiumcodereview.appspot.com/9370016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:406: private
static class TokenData implements Cloneable {
On 2012/02/09 15:44:06, zundel wrote:
> On 2012/02/09 12:31:42, kasperl wrote:
> > Does this need to be Cloneable?
> 
> No, not if we take out the super.clone() reference.  But Effecitve Java (Item
> 10) says this is the wrong thing to do if you are implementing clone() in a
> nonfinal class.  

I was trying to make the minimal set of changes, but you are correct that
removing Cloneable is the right way. 

In fact, it turns out that clone() was not being called for this class or
Location. Removed

https://chromiumcodereview.appspot.com/9370016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:412: protected
TokenData clone() {
On 2012/02/09 15:44:06, zundel wrote:
> We ditch clone and turn this into a copy constructor.
> 
> TokenData(TokenData orig) { ... }
> 
> On the one hand, we'll have to manually copy the fields.  On the other hand,
it
> would solve the performance problem without us having a clone() implementation
> that violates good advice.

It turns out this method is never called. Removed.

https://chromiumcodereview.appspot.com/9370016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/parser/DartScanner.java:413: TokenData
clone = new TokenData();
On 2012/02/09 15:44:06, zundel wrote:
> super.clone() is the right way to implement clone() acc'd to Josh Bloch which
> has to do with maintaining the relationship:  x.clone().getClass() == 
> x.getClass() if someone were to subclass TokenData
> 
> 
> We could also turn these classes into final classes, which solves the issue,
but
> is a bit subtle, we should certainly document it.

I was striving for a minimal set of changes, but you are correc that the right
way is to change this to a copy. Turns out it was not needed so I removed it
entirely.

[zundel, 2012-02-10 13:13:33]

lgtm