Skip to content

Commit

Permalink
565502: Poor performance with bad hashing on HashMapIntObject
Browse files Browse the repository at this point in the history 
Use new hash function, but still serialize in the
old format.

Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=565502

Change-Id: I6a8453921fa9e05382f45a038e8670d5c74e7f1f
  • Loading branch information
@ajohnson1
ajohnson1 committed Aug 17, 2020
1 parent c49b564 commit 08db308
Show file tree
Hide file tree
Showing 10 changed files with 613 additions and 106 deletions.
127 changes: 112 additions & 15 deletions plugins/org.eclipse.mat.report/src/org/eclipse/mat/collect/HashMapIntLong.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2008, 2018 SAP AG and IBM Corporation.
* Copyright (c) 2008, 2020 SAP AG and IBM Corporation.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
Expand Down Expand Up @@ -92,15 +92,15 @@ public boolean put(int key, long value)
resize(capacity <= BIG_CAPACITY >> 1 ? capacity << 1 : capacity < BIG_CAPACITY ? BIG_CAPACITY : capacity + 1);
}

int hash = (key & Integer.MAX_VALUE) % capacity;
int hash = hash(key);
while (used[hash])
{
if (keys[hash] == key)
{
values[hash] = value;
return true;
}
hash = (hash + step) % capacity;
hash = step(hash);
}
used[hash] = true;
keys[hash] = key;
Expand All @@ -110,14 +110,43 @@ public boolean put(int key, long value)
return false;
}

private int oldHash(int key)
{
return (key & Integer.MAX_VALUE) % capacity;
}

private int step(int hash)
{
hash += step;
// Allow for overflow
if (hash >= capacity || hash < 0)
hash -= capacity;
return hash;
}

/**
* Hash function.
* Constant is phi and should be odd.
* Capacity is positive, so 31 bits, so we carefully
* shift down and expand up to 64 bits before extracting
* the result.
* @param key
* @return
*/
private int hash(int key)
{
int r = (int)(((key * 0x9e3779b97f4a7c15L >>> 31) * capacity) >>> 33);
return r;
}

/**
* Remove an mapping from the map
* @param key the key to remove
* @return true if entry was found
*/
public boolean remove(int key)
{
int hash = (key & Integer.MAX_VALUE) % capacity;
int hash = hash(key);
while (used[hash])
{
if (keys[hash] == key)
Expand All @@ -126,24 +155,24 @@ public boolean remove(int key)
size--;
// Re-hash all follow-up entries anew; Do not fiddle with the
// capacity limit (75 %) otherwise this code may loop forever
hash = (hash + step) % capacity;
hash = step(hash);
while (used[hash])
{
key = keys[hash];
used[hash] = false;
int newHash = (key & Integer.MAX_VALUE) % capacity;
int newHash = hash(key);
while (used[newHash])
{
newHash = (newHash + step) % capacity;
newHash = step(newHash);
}
used[newHash] = true;
keys[newHash] = key;
values[newHash] = values[hash];
hash = (hash + step) % capacity;
hash = step(hash);
}
return true;
}
hash = (hash + step) % capacity;
hash = step(hash);
}

return false;
Expand All @@ -156,11 +185,11 @@ public boolean remove(int key)
*/
public boolean containsKey(int key)
{
int hash = (key & Integer.MAX_VALUE) % capacity;
int hash = hash(key);
while (used[hash])
{
if (keys[hash] == key) { return true; }
hash = (hash + step) % capacity;
hash = step(hash);
}
return false;
}
Expand All @@ -173,11 +202,11 @@ public boolean containsKey(int key)
*/
public long get(int key)
{
int hash = (key & Integer.MAX_VALUE) % capacity;
int hash = hash(key);
while (used[hash])
{
if (keys[hash] == key) { return values[hash]; }
hash = (hash + step) % capacity;
hash = step(hash);
}

throw noSuchElementException;
Expand Down Expand Up @@ -380,10 +409,10 @@ private void resize(int newCapacity)
if (oldUsed[i])
{
key = oldKeys[i];
hash = (key & Integer.MAX_VALUE) % capacity;
hash = hash(key);
while (used[hash])
{
hash = (hash + step) % capacity;
hash = step(hash);
}
used[hash] = true;
keys[hash] = key;
Expand All @@ -392,4 +421,72 @@ private void resize(int newCapacity)
}
size = oldSize;
}

/**
* Calculate a suitable initial capacity
* @return initial capacity which has the same capacity, step
*/
private int calcInit()
{
// calculate initial capacity for this capacity and step
int c2 = capacity - 1;
int c1c = PrimeFinder.findPrevPrime(capacity);
int c1s = (step + 1) * 3;
int c1 = Math.max(c1c, c1s);
for (int c = c1; c <= c2; ++c)
{
int s1 = Math.max(1, PrimeFinder.findPrevPrime(c / 3));
if (s1 == step)
return c;
}
return c2;
}

/**
* Return a serializable version of this HashMap.
* Previous versions of the code use an old hash function
* and deserialize the fields directly, so we must map
* the values to the correct position for the old hash function.
* @return a old-style HashMapIntLong only suitable for serialization
*/
private Object writeReplace() {
HashMapIntLong out = new HashMapIntLong(calcInit());
for (int i = 0; i < capacity; ++i)
{
if (used[i])
{
int key = keys[i];
int hash = out.oldHash(key);
while (out.used[hash])
{
hash = out.step(hash);
}
out.used[hash] = true;
out.keys[hash] = key;
out.values[hash] = values[i];
++out.size;
}
}
return out;
}

/**
* Previous versions serialized the object directly using
* an old hash function, and the current version does
* the same for compatibility,
* so rebuild allowing a new hash function.
* @return
*/
private Object readResolve()
{
HashMapIntLong out = new HashMapIntLong(calcInit());
for (int i = 0; i < capacity; ++i)
{
if (used[i])
{
out.put(keys[i], values[i]);
}
}
return out;
}
}
57 changes: 40 additions & 17 deletions plugins/org.eclipse.mat.report/src/org/eclipse/mat/collect/HashMapIntObject.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2008, 2018 SAP AG and IBM Corporation.
* Copyright (c) 2008, 2020 SAP AG and IBM Corporation.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
Expand Down Expand Up @@ -93,7 +93,7 @@ public E put(int key, E value)
resize(capacity <= BIG_CAPACITY >> 1 ? capacity << 1 : capacity < BIG_CAPACITY ? BIG_CAPACITY : capacity + 1);
}

int hash = (key & Integer.MAX_VALUE) % capacity;
int hash = hash(key);
while (used[hash])
{
if (keys[hash] == key)
Expand All @@ -102,7 +102,7 @@ public E put(int key, E value)
values[hash] = value;
return oldValue;
}
hash = (hash + step) % capacity;
hash = step(hash);
}
used[hash] = true;
keys[hash] = key;
Expand All @@ -111,14 +111,37 @@ public E put(int key, E value)
return null;
}

private int step(int hash)
{
hash += step;
if (hash >= capacity || hash < 0)
hash -= capacity;
return hash;
}

/**
* Hash function.
* Constant is phi and should be odd.
* Capacity is positive, so 31 bits, so we carefully
* shift down and expand up to 64 bits before extracting
* the result.
* @param key
* @return
*/
private int hash(int key)
{
int r = (int)(((key * 0x9e3779b97f4a7c15L >>> 31) * capacity) >>> 33);
return r;
}

/**
* Remove an mapping from the map
* @param key the key to remove
* @return the old value if the key was found, otherwise null
*/
public E remove(int key)
{
int hash = (key & Integer.MAX_VALUE) % capacity;
int hash = hash(key);
while (used[hash])
{
if (keys[hash] == key)
Expand All @@ -128,24 +151,24 @@ public E remove(int key)
size--;
// Re-hash all follow-up entries anew; Do not fiddle with the
// capacity limit (75 %) otherwise this code may loop forever
hash = (hash + step) % capacity;
hash = step(hash);
while (used[hash])
{
key = keys[hash];
used[hash] = false;
int newHash = (key & Integer.MAX_VALUE) % capacity;
int newHash = hash(key);
while (used[newHash])
{
newHash = (newHash + step) % capacity;
newHash = step(newHash);
}
used[newHash] = true;
keys[newHash] = key;
values[newHash] = values[hash];
hash = (hash + step) % capacity;
hash = step(hash);
}
return oldValue;
}
hash = (hash + step) % capacity;
hash = step(hash);
}
return null;
}
Expand All @@ -157,11 +180,11 @@ public E remove(int key)
*/
public boolean containsKey(int key)
{
int hash = (key & Integer.MAX_VALUE) % capacity;
int hash = hash(key);
while (used[hash])
{
if (keys[hash] == key) { return true; }
hash = (hash + step) % capacity;
hash = step(hash);
}
return false;
}
Expand All @@ -173,11 +196,11 @@ public boolean containsKey(int key)
*/
public E get(int key)
{
int hash = (key & Integer.MAX_VALUE) % capacity;
int hash = hash(key);
while (used[hash])
{
if (keys[hash] == key) { return values[hash]; }
hash = (hash + step) % capacity;
hash = step(hash);
}
return null;
}
Expand Down Expand Up @@ -414,10 +437,10 @@ private void resize(int newCapacity)
if (oldUsed[i])
{
key = oldKeys[i];
hash = (key & Integer.MAX_VALUE) % capacity;
hash = hash(key);
while (used[hash])
{
hash = (hash + step) % capacity;
hash = step(hash);
}
used[hash] = true;
keys[hash] = key;
Expand Down Expand Up @@ -458,15 +481,15 @@ private void readObject(ObjectInputStream stream) throws IOException, ClassNotFo

private void putQuick(int key, E value)
{
int hash = (key & Integer.MAX_VALUE) % capacity;
int hash = hash(key);
while (used[hash])
{
if (keys[hash] == key)
{
values[hash] = value;
return;
}
hash = (hash + step) % capacity;
hash = step(hash);
}
used[hash] = true;
keys[hash] = key;
Expand Down
Loading

0 comments on commit 08db308

Please sign in to comment.