From: Yann Simon <hidden> Date: 2016-06-15 22:46:25
From FindBugs:
Using the java.lang.String(String) constructor wastes memory because
the object so constructed will be functionally indistinguishable from
the String passed as a parameter. Just use the argument String directly.
Signed-off-by: Yann Simon <redacted>
---
.../src/org/spearce/jgit/lib/RefDatabase.java | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:46:25
Yann Simon [off-list ref] wrote:
quoted hunk
From FindBugs:
Using the java.lang.String(String) constructor wastes memory because
the object so constructed will be functionally indistinguishable from
the String passed as a parameter. Just use the argument String directly.
Signed-off-by: Yann Simon <redacted>
---
.../src/org/spearce/jgit/lib/RefDatabase.java | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
I had a specific reason for forcing a new String object here.
The line in question, p, is from the packed-refs file and
contains the entire SHA-1 in hex form at the beginning of it.
We've converted that into binary as an ObjectId, it uses 1/4 the
space of the string portion.
The Ref object, its ObjectId, and its name string, are going to be
cached in a Map, probably long-term. We're better off shedding the
80 bytes of memory used to hold the hex SHA-1 then risk substring()
deciding its "faster" to reuse the char[] then to make a copy of it.
--
Shawn.
From: Yann Simon <hidden> Date: 2016-06-15 22:46:25
2009/3/19 Shawn O. Pearce [off-list ref]:
The line in question, p, is from the packed-refs file and
contains the entire SHA-1 in hex form at the beginning of it.
We've converted that into binary as an ObjectId, it uses 1/4 the
space of the string portion.
The Ref object, its ObjectId, and its name string, are going to be
cached in a Map, probably long-term. We're better off shedding the
80 bytes of memory used to hold the hex SHA-1 then risk substring()
deciding its "faster" to reuse the char[] then to make a copy of it.
Thany you for the explanation.
I learn something, and my tests confirm it.
The p.substring(...) can keep the entire array of char by only
updating the intern offset value.
new String(p.subtring(...)) make sure that the variable contains only
the final chars.
Yann
From: Yann Simon <hidden> Date: 2016-06-15 22:47:02
2009/3/19 Shawn O. Pearce [off-list ref]:
Yann Simon [off-list ref] wrote:
quoted
From FindBugs:
Using the java.lang.String(String) constructor wastes memory because
the object so constructed will be functionally indistinguishable from
the String passed as a parameter. Just use the argument String directly.
Signed-off-by: Yann Simon <redacted>
---
.../src/org/spearce/jgit/lib/RefDatabase.java | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
final int sp = p.indexOf(' ');
final ObjectId id = ObjectId.fromString(p.substring(0, sp));
- final String name = new String(p.substring(sp + 1));
+ final String name = p.substring(sp + 1);
last = new Ref(Ref.Storage.PACKED, name, name, id);
newPackedRefs.put(last.getName(), last);
I had a specific reason for forcing a new String object here.
The line in question, p, is from the packed-refs file and
contains the entire SHA-1 in hex form at the beginning of it.
We've converted that into binary as an ObjectId, it uses 1/4 the
space of the string portion.
The Ref object, its ObjectId, and its name string, are going to be
cached in a Map, probably long-term. We're better off shedding the
80 bytes of memory used to hold the hex SHA-1 then risk substring()
deciding its "faster" to reuse the char[] then to make a copy of it.
However, using the trick newString = new String(aString.substring(),
i) does not work on all JVM.
With an IBM JVM, the newString will still contain the original array of chars.
Another solution that work on all JVM could be:
newString = new String(aString.substring(i).toCharArray())
Or
newString = new String(aString.toCharArray(), i, aString.length() - i)
I like the latter one.
Yann
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:47:02
From FindBugs:
Using the java.lang.String(String) constructor wastes memory
because the object so constructed will be functionally
indistinguishable from the String passed as a parameter. Just
use the argument String directly.
Actually, here we want to get a new String object that covers only
the portion of the source string that we are selected out.
The line in question, p, is from the packed-refs file and contains
the entire SHA-1 in hex form at the beginning of it. We have already
converted that into binary as an ObjectId, which uses 1/4 the space
of the string portion.
The Ref object, its ObjectId, and its name string, are going to be
cached in a Map, probably long-term, as the packed-refs file does
not change frequently. We are better off shedding the 80 bytes of
memory used to hold the hex SHA-1 then risk substring() deciding its
"better" to reuse the same char[] internally.
By creating a new StringBuilder of the exact required capacity for
the name, and then copying in the region of characters we really
want, we defeat the reuse substring() would normally perform, at
the tiny cost of an extra StringBuilder temporary. Some JITs are
able to stack allocate that here, making it a trivial cost.
Signed-off-by: Shawn O. Pearce <redacted>
CC: Yann Simon <redacted>
---
Yann Simon [off-list ref] wrote:
> 2009/3/19 Shawn O. Pearce [off-list ref]:
> > Yann Simon [off-list ref] wrote:
> >> From FindBugs:
> >> Using the java.lang.String(String) constructor wastes [...]
>
> However, using the trick newString = new String(aString.substring(),
> i) does not work on all JVM.
> With an IBM JVM, the newString will still contain the original array of chars.
>
> Another solution that work on all JVM could be:
> newString = new String(aString.substring(i).toCharArray())
> Or
> newString = new String(aString.toCharArray(), i, aString.length() - i)
>
> I like the latter one.
I prefer this. It should always do what we want, and at a lower
temporary memory footprint (one less copy of the name). IIRC Robin
rejected it earlier because it wasn't obvious what we were doing. I
say hogwash, its clear as mud.
.../src/org/spearce/jgit/lib/RefDatabase.java | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
From: Yann Simon <hidden> Date: 2016-06-15 22:47:03
2009/7/10 Shawn O. Pearce [off-list ref]:
Yann Simon [off-list ref] wrote:
>
> However, using the trick newString = new String(aString.substring(),
> i) does not work on all JVM.
> With an IBM JVM, the newString will still contain the original array of chars.
>
> Another solution that work on all JVM could be:
> newString = new String(aString.substring(i).toCharArray())
> Or
> newString = new String(aString.toCharArray(), i, aString.length() - i)
>
> I like the latter one.
I prefer this. It should always do what we want, and at a lower
temporary memory footprint (one less copy of the name). IIRC Robin
rejected it earlier because it wasn't obvious what we were doing. I
say hogwash, its clear as mud.
+ private static String copy(final String src, final int off, final int end) {
+ return new StringBuilder(end - off).append(src, off, end).toString();
+ }
+
This method is quite clear.
One line javadoc would make it even clearer... :p (and maybe make Robin happy)
And you're right: by using a StringBuilder, we need one less arraycopy.
After committing your change, we can remove the entry to silent FindBugs.
(commit 21c3d82824075cd1f140b3bcf252dfaffe0fc96c)
Yann
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:47:03
From FindBugs:
Using the java.lang.String(String) constructor wastes memory
because the object so constructed will be functionally
indistinguishable from the String passed as a parameter. Just
use the argument String directly.
Actually, here we want to get a new String object that covers only
the portion of the source string that we are selected out.
The line in question, p, is from the packed-refs file and contains
the entire SHA-1 in hex form at the beginning of it. We have already
converted that into binary as an ObjectId, which uses 1/4 the space
of the string portion.
The Ref object, its ObjectId, and its name string, are going to be
cached in a Map, probably long-term, as the packed-refs file does
not change frequently. We are better off shedding the 80 bytes of
memory used to hold the hex SHA-1 then risk substring() deciding its
"better" to reuse the same char[] internally.
By creating a new StringBuilder of the exact required capacity for
the name, and then copying in the region of characters we really
want, we defeat the reuse substring() would normally perform, at
the tiny cost of an extra StringBuilder temporary. Some JITs are
able to stack allocate that here, making it a trivial cost.
Signed-off-by: Shawn O. Pearce <redacted>
CC: Yann Simon <redacted>
---
Yann Simon [off-list ref] wrote:
> This method is quite clear.
> One line javadoc would make it even clearer... :p (and maybe make Robin happy)
Javadoc is overrated. Private utility methods like this that are one
line long don't need documentation. The rationale for why this line
does what it does is something that `git blame` can answer better.
> And you're right: by using a StringBuilder, we need one less arraycopy.
>
> After committing your change, we can remove the entry to silent FindBugs.
> (commit 21c3d82824075cd1f140b3bcf252dfaffe0fc96c)
My patch is updated (below). Thanks, I forgot about that filter.
.../findBugs/FindBugsExcludeFilter.xml | 7 -------
.../src/org/spearce/jgit/lib/RefDatabase.java | 6 +++++-
2 files changed, 5 insertions(+), 8 deletions(-)
From: Robin Rosenberg <hidden> Date: 2016-06-15 22:47:04
måndag 13 juli 2009 16:53:08 skrev "Shawn O. Pearce" [off-list ref]:
Yann Simon [off-list ref] wrote:
> This method is quite clear.
> One line javadoc would make it even clearer... :p (and maybe make Robin happy)
Javadoc is overrated. Private utility methods like this that are one
line long don't need documentation. The rationale for why this line
does what it does is something that `git blame` can answer better.
Sorry Yann, I'm with Shawn here. A comment wouldn't add anything here since
the method is pretty simple. Besides javadocs are for API's so an inline comment
would be better, but I don't think it is necessary.
Shawn, any references for the ability of JIT's abilities to stack allocate in this context? For
me learning, will commit anyway.
-- robina
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:47:04
Robin Rosenberg [off-list ref] wrote:
Shawn, any references for the ability of JIT's abilities to stack allocate in this context? For
me learning, will commit anyway.
See [1] for example. I read a presentation from a HotSpot engineer
at Sun a year or two ago that talked about it as a feature in the
Sun Java 6 runtime, but I can't track that down now.
Its a pretty simple concept. Folks realized that some types,
e.g. StringBuilder, are often used only within a single stack
frame, and that escape analysis can be used to prove that the
StringBuilder instance is only visible within that stack frame.
Doing a stack allocation instead of a heap allocation would allow
the JVM to avoid creating unnecessary garbage.
Ah, according to [2] the feature is only in 6u14 and later, and is
an option you still need to enable on the command line. But its
the direction the Sun JVM is going. I imagine it would be on by
default in the future, various performance tests seem to indicate
its a worthwhile optimization.
[1] http://www.ibm.com/developerworks/java/library/j-jtp09275.html?ca=dgr-lnxw01JavaUrbanLegends
[2] http://java.sun.com/javase/6/webnotes/6u14.html
--
Shawn.
From: Robin Rosenberg <hidden> Date: 2016-06-15 22:47:04
tisdag 21 juli 2009 17:03:37 skrev "Shawn O. Pearce" [off-list ref]:
Robin Rosenberg [off-list ref] wrote:
quoted
Shawn, any references for the ability of JIT's abilities to stack allocate in this context? For
me learning, will commit anyway.
See [1] for example. I read a presentation from a HotSpot engineer
at Sun a year or two ago that talked about it as a feature in the
Sun Java 6 runtime, but I can't track that down now.