gmane.comp.java.openjdk.core-libs.devel

Re: Review Request CR#7118743 : Alternative Hashing for String withHash-based Maps

Ulf Zibis — 2012-05-26T01:43:40

Am 23.05.2012 07:16, schrieb Mike Duigou:

To me it seems, that computing the murmur hash is more expensive, especially on short strings, than 
the old hash algorithm.
So I think, a developer should have an influence on the hash algorithm to be used by a hash map, 
especially for strings, see http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6812862.

So I propose a public accessible field in String:
     public int customHash;

Then the HashMap should have an overrideable hash method:
     protected int hash(K key) {
         h = key.hashCode();
         h ^= (h >>> 20) ^ (h >>> 12);
         return h ^ (h >>> 7) ^ (h >>> 4);
     }

Then we should have a new class:
public class MurmurHashMap<V> extends HashMap<String,V> {
     protected final int hash(String key) {
         int h = key.customHash;
         if (h == 0) {
             // harmless data race on customHash here.
             h = Hashing.murmur3_32(HASHING_SEED, key);
             // ensure result is not zero to avoid recalculating

Re: Review request for CR 171917 CachedRowSetImpl.populate does nothandle map properly

Lance Andersen - Oracle — 2012-05-25T23:43:21

Hi Dave,

No, I should be able to make that change.  Thanks for the suggestion.  Once I do and rerun the unit tests, jck and sqe, I will send the diff back out.

Best
Lance
On May 25, 2012, at 7:35 PM, David Schlosnagle wrote:



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org

Re: Review request for CR 171917 CachedRowSetImpl.populate does nothandle map properly

David Schlosnagle — 2012-05-25T23:35:56

On Fri, May 25, 2012 at 3:50 PM, Lance Andersen - Oracle
<Lance.Andersen-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org> wrote:

Lance,

Is there any reason not to use Map.isEmpty() which would be useful if
the Map has an expensive size() method?

 -                if (map == null) {
 +                if (map == null || map.isEmpty()) {

Thanks,
Dave

Re: Re : Review Request CR#7118743 : Alternative Hashing for Stringwith Hash-based Maps [private fields]

Vitaly Davidovich — 2012-05-25T22:36:27

Agreed on JDK code, but since accessors are also generated for access to
outer class' private fields/methods from inner class, wouldn't want people
to increase visibility of those fields/methods just to get rid of the
accessors in their own code thinking that there are serious problems
associated with them.

Anyway, we're in agreement. :)

Sent from my phone
On May 25, 2012 6:18 PM, "Rémi Forax" <forax-TRAJqka12BNWj0EZb7rXcA< at >public.gmane.org> wrote:

Re: Request for Review : CR#6924259: Remove String.count/String.offset

Ulf Zibis — 2012-05-25T22:36:06

Am 25.05.2012 17:05, schrieb Jim Gish:

I believe IDE implementers refer to the "Code Conventions for the JavaTM Programming Language", 8.2 
by default.
So simply fixing this bug would help:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6939278

-Ulf

Re: Request for Review : CR#6924259: Remove String.count/String.offset

Rémi Forax — 2012-05-25T22:33:21

I may be wrong but this constructor is used rarely
so for me it's like you add one static field that will be never used.

[...]


duh, sorry, I've forgotten the basics. You're right.


regards,
Rémi

Re: Re : Review Request CR#7118743 : Alternative Hashing for Stringwith Hash-based Maps [private fields]

Rémi Forax — 2012-05-25T22:21:16

I'm sure it's not used but it can be.


cheers,
Rémi

Re: Re : Review Request CR#7118743 : Alternative Hashing for Stringwith Hash-based Maps [private fields]

Vitaly Davidovich — 2012-05-25T22:17:58

I assume you're referring to instanceKlass::_host_klass? The comment on
that field says it's only non-null for an anonymous class, no mention of
named inner classes.  Are you sure it's used for named inner classes to
point back to their parent?

Cheers

Sent from my phone
On May 25, 2012 6:06 PM, "Rémi Forax" <forax-TRAJqka12BNWj0EZb7rXcA< at >public.gmane.org> wrote:

Re: Re : Review Request CR#7118743 : Alternative Hashing for Stringwith Hash-based Maps [private fields]

Rémi Forax — 2012-05-25T22:20:26

given that this code is in java.lang, thus use by millions,
I have no problem to agree that when I review this kind of code,
I switch myself in pedandic-mode.


cheers,
Rémi

Re: Re : Review Request CR#7118743 : Alternative Hashing for Stringwith Hash-based Maps [private fields]

Vitaly Davidovich — 2012-05-25T22:09:36

Yes, methods/constructors as well - I should've pointed that out but only
answered the field question that was originally asked.

Valid points on metadata and stack pollution, although a bit pedantic for
real code :).

Thanks

Sent from my phone
On May 25, 2012 6:03 PM, "Rémi Forax" <forax-TRAJqka12BNWj0EZb7rXcA< at >public.gmane.org> wrote:

Re: Request for Review : CR#6924259: Remove String.count/String.offset

Mike Duigou — 2012-05-25T22:09:28

On May 25 2012, at 06:57 , Rémi Forax wrote:


I have removed most of the use of EMPTY_STRING_VALUE. It remains for the empty constructor only.


Reasonable and confirmed with Valdimir Kozlov.


Good optimization. I had to add a sync around the buffer version.


none. I will leave for a later change.


Restored but deprecated.


I don't see where either of them need to do any checking.


Done.


Reasonable. I opted not to add the 3rd variant ([] count) because it wasn't used. I've reverted to using the 3 param version.

Re: Re : Review Request CR#7118743 : Alternative Hashing for Stringwith Hash-based Maps [private fields]

Rémi Forax — 2012-05-25T22:08:28

BTW, the VM knows what an anonymous class is
(there is an host class field in the runtime class meta-data),
it's more a class format issue now, i.e. the class format
doesn't support to put more than one class in a .class
classfile.

Rémi

Re: Re : Review Request CR#7118743 : Alternative Hashing for Stringwith Hash-based Maps [private fields]

Rémi Forax — 2012-05-25T22:04:52

and methods and constructors


yes, there are unconditionally inlined so performance is not the problem.
The issues are more that the compiler generates unnecessary code,
unnecessary class metadata, stack pollution and stacktrace pollution too.

Rémi

Re: Re : Review Request CR#7118743 : Alternative Hashing for Stringwith Hash-based Maps [private fields]

Vitaly Davidovich — 2012-05-25T21:55:34

In case it's not clear, by JVM not allowing them I simply mean that it
doesn't know about nested classes (not a first class concept), so since
it's emulated by javac, javac creates these package private synthetic
accessors.

Sent from my phone
On May 25, 2012 5:50 PM, "Vitaly Davidovich" <vitalyd-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:

Re: Re : Review Request CR#7118743 : Alternative Hashing for Stringwith Hash-based Maps [private fields]

Vitaly Davidovich — 2012-05-25T21:50:06

This is specific to private fields in inner classes - java allows access to
them from the outer class, but the JVM doesn't so javac generates synthetic
accessor methods for them.  Don't think it's a problem for JIT compiler
though as it should inline them.

Sent from my phone
On May 25, 2012 5:23 PM, "Ulf Zibis" <Ulf.Zibis-Mmb7MZpHnFY< at >public.gmane.org> wrote:

Re: Re : Review Request CR#7118743 : Alternative Hashing for Stringwith Hash-based Maps [private fields]

Ulf Zibis — 2012-05-25T21:22:31

Am 25.05.2012 22:30, schrieb Jeff Hain:
I'm interested too.

-Ulf

Re : Review Request CR#7118743 : Alternative Hashing for String withHash-based Maps [private fields]

Jeff Hain — 2012-05-25T20:30:55

Hello.


I wasn't aware that making fields private could have a downside
(other than making them non-visible).

Could you, or anyone, please give (a link to) more info about this?


Thanks.

-Jeff

Review request CR 7171918 XmlReaderContentHandler.endElement does nothandle a Delete Tag properly

Lance Andersen - Oracle — 2012-05-25T19:56:41

Attached is the fix for 7171918.   If a WebRowSet is written to disk that contains a row marked for deletion and then read back into a WebRowSet, it was not marked again as a deleted row.

Here is the fix to endElement() to address this.

 hg diff XmlReaderContentHandler.java 
diff -r 4580652d9828 src/share/classes/com/sun/rowset/internal/XmlReaderContentHandler.java
--- a/src/share/classes/com/sun/rowset/internal/XmlReaderContentHandler.javaFri May 04 16:00:47 2012 -0400
+++ b/src/share/classes/com/sun/rowset/internal/XmlReaderContentHandler.javaFri May 25 15:52:21 2012 -0400
< at >< at > -764,6 +764,7 < at >< at >
                     rs.next();
                     rs.setOriginalRow();
                     applyUpdates();
+                    rs.deleteRow();
                 } catch (SQLException ex) {
                     throw new SAXException(MessageFormat.format(resBundle.handleGetObject("xmlrch.errdel").toString() , ex.getMessage()));


Best,
Lance


Lance Andersen| Principal Member of Technical Staff | +1.781.442.203

Review request for CR 171917 CachedRowSetImpl.populate does nothandle map properly

Lance Andersen - Oracle — 2012-05-25T19:50:39

Attached is the fix for bug 171917 .

The populate() method needs to check for a size of 0 for the map in case a webrowset xml file has an empty map tag,  which would result in calling setobject specifying a map and not all databases/drivers support this.

simple 1 line change:

hg diff CachedRowSetImpl.java 
diff -r 4580652d9828 src/share/classes/com/sun/rowset/CachedRowSetImpl.java
--- a/src/share/classes/com/sun/rowset/CachedRowSetImpl.javaFri May 04 16:00:47 2012 -0400
+++ b/src/share/classes/com/sun/rowset/CachedRowSetImpl.javaFri May 25 15:45:29 2012 -0400
< at >< at > -659,7 +659,7 < at >< at >
                  * us work with drivers that do not support
                  * getObject with a map in fairly sensible way
                  */
-                if (map == null) {
+                if (map == null || map.size() == 0) {
                     obj = data.getObject(i);
                 } else {
                     obj = data.getObject(i, map);


Best
Lance

Lance Andersen| Principal Member of Technical Staff | +1.7

Re: Review Request CR#7118743 : Alternative Hashing for String withHash-based Maps

Rémi Forax — 2012-05-25T17:38:53

Hello Mike,
just two small issues.

In WeakHashMap.hash(), the null check should be done at callsite,
the call to hash32() should be directly returned like in HashMap.

Is Hashable32 is used anymore ?

Rémi

Re: Request for Review : CR#6924259: Remove String.count/String.offset

David Schlosnagle — 2012-05-25T17:24:46

Hi Mike,

I had a few quick comments that I didn't see exactly covered so far:

Should we extract original.value to a local to avoid multiple getfield
instructions in the non-empty case?

    public String(String original) {
        char[] originalValue = original.value;
        this.value = (originalValue.length > 0)
                ? originalValue
                : EMPTY_STRING_VALUE;
        this.hash = original.hash;
    }


Should the len be moved outside of the if condition in the following, and use
Arrays.copyOf (or value.clone()) to simplify the code and hopefully be
intrinsified?

    public String(char value[]) {
        final int len = value.length;
        this.value = (len > 0)
                ? Arrays.copyOf(value, len);
                : EMPTY_STRING_VALUE;
    }

[1] http://markmail.org/message/ubg6s5dyvsidjipj

Thanks,
Dave