286318 - Optimize frequently called function pk11_SessionFromHandle

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

pk11_SessionFromHandle is called a lot.  We should
make it run as fast as possible.

Comment 1

[Wan-Teh Chang]

Attachment: Save the result of PK11_SESSION_LOCK

During the pk11wrap/softoken code review meeting,
I wrote down on my notebook "optimize pk11_SessionFromHandle".
That's why I filed this bug report.

Unfortunately, looking at the code now, I no longer
remember what optimizations I had in mind.

The only optimization I remember is to save the
result of PK11_SESSION_LOCK so we don't call the
macro twice.  This problem is in many other functions,
so I fix them all in this patch.

This patch needs to be reviewed carefully, especially
if the original PK11_SESSION_LOCK calls are inside
a loop.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 177554 [details] [diff] [review]
Save the result of PK11_SESSION_LOCK

Given that PKCS11_USE_THREADS is always defined, and that 
PK11_USE_THREADS(x) always expands to x
this patch is correct.	

If there was ever a chance that PKCS11_USE_THREADS would become undefined,
and consequently PK11_USE_THREADS(x) would expand to empty, then this 
patch's new code would be assigning a value to an unused lock variable.

I'd prefer to see all the PK11_USE_THREADS macros eliminated, making their
calls to PZ_Lock and PZ_Unlock an unconditional part of the code.  
But that can be done in a separate patch.  

r=nelson

Comment 3

[Wan-Teh Chang]

Comment on attachment 177554 [details] [diff] [review]
Save the result of PK11_SESSION_LOCK

Good point, Nelson.  I missed that.

I can put the declarations of the 'lock'
variable in PK11_USE_THREADS().

I'll open a new bug to remove PK11_USE_THREADS(x).

Comment 4

[Wan-Teh Chang]

Attachment: Save the result of PK11_SESSION_LOCK, v1.1

I put all relevant code in PK11_USE_THREADS() per
Nelson's review comment.

Comment 5

[Wan-Teh Chang]

I checked in the PK11_SESSION_LOCK patch, v1.1, on
the NSS trunk for NSS 3.10.

I'm marking the bug fixed.  Please reopen if you see
any other optimization we can do to pk11_SessionFromHandle.

Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.97; previous revision: 1.96
done
Checking in pkcs11u.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.63; previous revision: 1.62
done

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Please don't increase the use of PK11_USE_THREADS! 
Please eliminate it.  It doesn't have to all be eliminatged at once.
Please?

Comment 7

[Wan-Teh Chang]

I want to leave the code in a consistent state.
The code is using PK11_USE_THREADS now.  So
my new code also uses that.  Whether I think the
macro is necessary is irrelevant.

I will attach a patch to bug 286439 next to remove
PK11_USE_THREADS.

Comment 8

[Robert Relyea]

Comment on attachment 177628 [details] [diff] [review]
Save the result of PK11_SESSION_LOCK, v1.1

The remove PK11_USE_THREADS is a subject of another bug.

r+ (for already checked in patch;)