Hash twice

Started by Simon Riggsabout 13 years ago3 messages
#1Simon Riggs
simon@2ndQuadrant.com
1 attachment(s)

Lock code says it calculates "hash value once and then pass around as needed".

But it actually calculates it twice for new locks.

Trivial patch attached to make it follow the comments in
LockTagHashCode and save a few cycles.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachments:

hash_once.v1.patchapplication/octet-stream; name=hash_once.v1.patchDownload
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 5f2239f..55e2fea 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -729,8 +729,12 @@ LockAcquireExtended(const LOCKTAG *locktag,
 	localtag.lock = *locktag;
 	localtag.mode = lockmode;
 
-	locallock = (LOCALLOCK *) hash_search(LockMethodLocalHash,
-										  (void *) &localtag,
+	/*
+	 * Calculate hash value once and then pass around as needed
+	 */
+	hashcode = LockTagHashCode(&(localtag.lock));
+	locallock = (LOCALLOCK *) hash_search_with_hash_value(LockMethodLocalHash,
+										  (void *) &localtag, hashcode,
 										  HASH_ENTER, &found);
 
 	/*
@@ -740,7 +744,7 @@ LockAcquireExtended(const LOCKTAG *locktag,
 	{
 		locallock->lock = NULL;
 		locallock->proclock = NULL;
-		locallock->hashcode = LockTagHashCode(&(localtag.lock));
+		locallock->hashcode = hashcode;
 		locallock->nLocks = 0;
 		locallock->numLockOwners = 0;
 		locallock->maxLockOwners = 8;
@@ -763,7 +767,6 @@ LockAcquireExtended(const LOCKTAG *locktag,
 			locallock->maxLockOwners = newsize;
 		}
 	}
-	hashcode = locallock->hashcode;
 
 	/*
 	 * If we already hold the lock, we can just increase the count locally.
#2Robert Haas
robertmhaas@gmail.com
In reply to: Simon Riggs (#1)
Re: Hash twice

On Sat, Jan 12, 2013 at 11:39 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Lock code says it calculates "hash value once and then pass around as needed".

But it actually calculates it twice for new locks.

Trivial patch attached to make it follow the comments in
LockTagHashCode and save a few cycles.

Hmm. This is a nice idea, but it doesn't look right to me, because
you're searching LockMethodLocalHash with a hash code intended to be
used in LockMethodLockHash, and the two hashing schemes are not
compatible, because the former is hashing a LOCALLOCKTAG, and the
latter is hashing a LOCKTAG, and both are just using tag_hash.

On the flip side if I'm wrong and the hashing schemes are compatible,
there are other places in the file where the same trick could be
employed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Simon Riggs
simon@2ndQuadrant.com
In reply to: Robert Haas (#2)
Re: Hash twice

On 14 January 2013 19:12, Robert Haas <robertmhaas@gmail.com> wrote:

On Sat, Jan 12, 2013 at 11:39 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Lock code says it calculates "hash value once and then pass around as needed".

But it actually calculates it twice for new locks.

Trivial patch attached to make it follow the comments in
LockTagHashCode and save a few cycles.

Hmm. This is a nice idea, but it doesn't look right to me, because
you're searching LockMethodLocalHash with a hash code intended to be
used in LockMethodLockHash, and the two hashing schemes are not
compatible, because the former is hashing a LOCALLOCKTAG, and the
latter is hashing a LOCKTAG, and both are just using tag_hash.

You're right. At local level we need to refcount requests, whereas we
only ever pass first request through to main table. That means the
unique key is different.

On the flip side if I'm wrong and the hashing schemes are compatible,
there are other places in the file where the same trick could be
employed.

But having said that, we already make ProcLockHash use a variation of
the LockHash to avoid recalculation.

So we should just make
LocalLockTagHashCode = LockTagHashCode() + mode;

Then we can use LockTagHashCode everywhere, which is easier to do than
documenting why we don't.

Anyway, just an idle thought while looking into something else.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers