Request for Reviews 7173918&7173919: Small follow-ons to alternative hashing

2012-06-03 Thread Mike Duigou 
Two small issues related to the larger alternative string hashing changes 
(CR#7126277) from last week. One issue is for JDK 7 and the other is for JDK 8. 
Both are quite small.

For the JDK 7 issue, as documented in the review request, the default threshold 
for the alternative hashing in that patch was set to the minimum to 
unconditionally enable the feature. This was done to make compatibility testing 
easier. Developers are strongly encouraged to test their applications with the 
7u6b13 build to ensure that their applications do not later encounter problems. 
For the release version of 7u6 the default threshold will be set to 512. This 
patch sets that default and marks one field final.

http://cr.openjdk.java.net/~mduigou/7173918/0/webrev/

The JDK8 issue is number of small performance enhancements suggested by Ulf 
Zibis and Remi Forax. These changes are being considered now while this issue 
remains fresh in the maintainer and reviewer's minds.

http://cr.openjdk.java.net/~mduigou/7173919/0/webrev/

Thanks,

Mike

Re: [doc]Small modification on the WeakHashMap doc

2012-06-03 Thread Mike Duigou 
The change looks good to me.
On Jun 3 2012, at 20:05 , David Holmes wrote:

> On 4/06/2012 11:55 AM, Charles Lee wrote:
>> Thanks David. Do I need another review?
> 
> Yes. Someone from TL - Mike or Alan most likely.
> 
> David
> 
>> On 06/03/2012 06:15 AM, David Holmes wrote:
>>> Hi Charles,
>>> 
>>> I have no problem with this clarification in the implementation notes
>>> being added. I've checked with Joe and it does not require CCC approval.
>>> 
>>> David
>>> -
>>> 
>>> On 28/05/2012 5:36 PM, Charles Lee wrote:
 
 Hi devs,
 
 I'd like to propose a new minor change for the WeakHashMap doc, which I
 got it from David :-)
 
 Would anyone got some time to take a look this fix[1]?
 
 1. http://cr.openjdk.java.net/~littlee/7166055/webrev.01/
 
 
 On 05/07/2012 11:45 AM, David Holmes wrote:
> Hi Charles,
> 
> On 7/05/2012 1:05 PM, Charles Lee wrote:
>> Does anyone interested in this issue?
> 
> Interest and time are two different things :)
> 
> A shorter form would be:
> 
> "If the values in the map do not rely on the map holding strong
> references to them, then one way to deal with this is ...
> 
> David
> 
>> On 05/03/2012 02:52 PM, Charles Lee wrote:
>>> Hi guys,
>>> 
>>> In the Implementation notes of WeakHashMap[1], says:
>>> 
>>> /One way to deal with this is to wrap values themselves within
>>> WeakReferences before inserting, as in: m.put(key, new
>>> WeakReference(value)), and then unwrapping upon each get./
>>> 
>>> However, it is not concise and a little misleading. Because the value
>>> in the WeakReference can be GC'd if there are no strong reference to
>>> it. This behaviour surprises some customers.
>>> How about add a statement like [2]:
>>> 
>>> /However, as the use of WeakReference in this manner will not prevent
>>> value objects from being GC'd, this approach is only useful when
>>> entries in the map are not relied upon for keeping the underlying
>>> value objects "live"./
>>> 
>>> 
>>> 
>>> 
>>> [1]:
>>> http://docs.oracle.com/javase/7/docs/api/java/util/WeakHashMap.html
>>> [2]: http://cr.openjdk.java.net/~littlee/7166055/webrev.00/
>>> 
>>> 
>> 
>> 
> 
 
 
 --
 Yours Charles
 
>>> 
>> 
>>

Re: [doc]Small modification on the WeakHashMap doc

2012-06-03 Thread David Holmes 

On 4/06/2012 11:55 AM, Charles Lee wrote:

Thanks David. Do I need another review?


Yes. Someone from TL - Mike or Alan most likely.

David


On 06/03/2012 06:15 AM, David Holmes wrote:

Hi Charles,

I have no problem with this clarification in the implementation notes
being added. I've checked with Joe and it does not require CCC approval.

David
-

On 28/05/2012 5:36 PM, Charles Lee wrote:


Hi devs,

I'd like to propose a new minor change for the WeakHashMap doc, which I
got it from David :-)

Would anyone got some time to take a look this fix[1]?

1. http://cr.openjdk.java.net/~littlee/7166055/webrev.01/


On 05/07/2012 11:45 AM, David Holmes wrote:

Hi Charles,

On 7/05/2012 1:05 PM, Charles Lee wrote:

Does anyone interested in this issue?


Interest and time are two different things :)

A shorter form would be:

"If the values in the map do not rely on the map holding strong
references to them, then one way to deal with this is ...

David


On 05/03/2012 02:52 PM, Charles Lee wrote:

Hi guys,

In the Implementation notes of WeakHashMap[1], says:

/One way to deal with this is to wrap values themselves within
WeakReferences before inserting, as in: m.put(key, new
WeakReference(value)), and then unwrapping upon each get./

However, it is not concise and a little misleading. Because the value
in the WeakReference can be GC'd if there are no strong reference to
it. This behaviour surprises some customers.
How about add a statement like [2]:

/However, as the use of WeakReference in this manner will not prevent
value objects from being GC'd, this approach is only useful when
entries in the map are not relied upon for keeping the underlying
value objects "live"./




[1]:
http://docs.oracle.com/javase/7/docs/api/java/util/WeakHashMap.html
[2]: http://cr.openjdk.java.net/~littlee/7166055/webrev.00/










--
Yours Charles

Re: [doc]Small modification on the WeakHashMap doc

2012-06-03 Thread Charles Lee 

Thanks David. Do I need another review?

On 06/03/2012 06:15 AM, David Holmes wrote:

Hi Charles,

I have no problem with this clarification in the implementation notes 
being added. I've checked with Joe and it does not require CCC approval.


David
-

On 28/05/2012 5:36 PM, Charles Lee wrote:


Hi devs,

I'd like to propose a new minor change for the WeakHashMap doc, which I
got it from David :-)

Would anyone got some time to take a look this fix[1]?

1. http://cr.openjdk.java.net/~littlee/7166055/webrev.01/


On 05/07/2012 11:45 AM, David Holmes wrote:

Hi Charles,

On 7/05/2012 1:05 PM, Charles Lee wrote:

Does anyone interested in this issue?


Interest and time are two different things :)

A shorter form would be:

"If the values in the map do not rely on the map holding strong
references to them, then one way to deal with this is ...

David


On 05/03/2012 02:52 PM, Charles Lee wrote:

Hi guys,

In the Implementation notes of WeakHashMap[1], says:

/One way to deal with this is to wrap values themselves within
WeakReferences before inserting, as in: m.put(key, new
WeakReference(value)), and then unwrapping upon each get./

However, it is not concise and a little misleading. Because the value
in the WeakReference can be GC'd if there are no strong reference to
it. This behaviour surprises some customers.
How about add a statement like [2]:

/However, as the use of WeakReference in this manner will not prevent
value objects from being GC'd, this approach is only useful when
entries in the map are not relied upon for keeping the underlying
value objects "live"./




[1]:
http://docs.oracle.com/javase/7/docs/api/java/util/WeakHashMap.html
[2]: http://cr.openjdk.java.net/~littlee/7166055/webrev.00/










--
Yours Charles






--
Yours Charles

Re: hg: jdk8/tl/jdk: 6924259: Remove offset and count fields from java.lang.String

2012-06-03 Thread Mike Duigou 
[I trimmed the distribution list]

On Jun 3 2012, at 13:44 , Peter Levart wrote:

> On Thursday, May 31, 2012 03:22:35 AM mike.dui...@oracle.com wrote:
>> Changeset: 2c773daa825d
>> Author:mduigou
>> Date:  2012-05-17 10:06 -0700
>> URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2c773daa825d
>> 
>> 6924259: Remove offset and count fields from java.lang.String
>> Summary: Removes the use of shared character array buffers by String along
>> with the two fields needed to support the use of shared buffers.
> 
> Wow, that's quite a change.

Indeed. It was a long time in development. It is a change which is expected to 
be overall beneficial though and in the general case a positive win. 

> So .substring() is not O(1) any more?

No. Though with object allocation it probably was only ever roughly O(1) anyway.

> Doesn't this have impact on the performance of parsers and such that rely on 
> the performance caracteristics of the .substring() ?

It does have an impact. We've seen as much as a couple of percent on some 
benchmarks. Parsers which use substring for extraction are definitely impacted 
by this change.

> Have you considered then implementing .subSequence() not in terms of just 
> delegating to .substring() but returning a special CharSequence view over the 
> chars of the sub-sequence?

It does look that String.subSequence() returning a special view rather than a 
substring would be a good optimization and probably a very good compromise for 
parser developers. Please create an issue and if you have the time and 
expertise a patch would speed things along (though unfortunately almost 
certainly too late for inclusion in 7u6).

Mike

> Regards, Peter
> 
>> Reviewed-by: alanb, mduigou, forax, briangoetz
>> Contributed-by: brian.dohe...@oracle.com
>> 
>> ! src/share/classes/java/lang/Integer.java
>> ! src/share/classes/java/lang/Long.java
>> ! src/share/classes/java/lang/String.java
>> ! src/share/classes/java/lang/StringCoding.java