Ian McGreer Reporter
	Description • 22 years ago

One of the complaints about NSS performance has been the number of memcpy calls
required to process and SSL request.  In investigating this issue, I have found
that NSS calls memcpy ~1200 times per request, and ~120 (10%) of those are
related to the handling of PKCS#11 object attributes.

In the softoken, object attributes are stored in a fixed-sized array that is
part of PK11ObjectStr.  This array is treated in essentially the same way as a
template of attributes (CK_ATTRIBUTE_PTR).  This makes handling of attributes
fairly simple, and all object types (certs, keys) are handled the same way. 
However, this generic method means that every time an object is created, all of
its attributes (including defaults) must be malloc'ed and memcpy'ed into buffers
in the fixed-size table.  Those operations are rather expensive.  Most of the
time, the attributes being copied are booleans (CK_BBOOL, a char) and integers
(CK_ULONG, a uint), which means a lot of 1-byte and 4-byte mallocs/memcpys.

They are also unnecessary.  The PKCS#11 standard clearly defines which
attributes may be associated with a given object type.  Thus, if each object
type had a structure containing all of its attributes, object creation would
involve assignments into known types, instead of generic buffers.  For example,
a key object would include a CK_BBOOL for the CKA_DERIVE attribute, and when
C_DeriveKey is called, the key object would just assign into that CK_BBOOL,
instead of allocating a single byte and using memcpy.

In SSL, the only objects (generally) created on the server side are secret keys.
 I have implemented a patch that transfers attribute handling for secret key
objects from the generic PK11AttributeStr to a secretKeyObjectAttrsStr.  I found
that while this was a substantial amount of code, it wasn't prohibitive.  But
secret keys are fairly simple, certs are much more difficult.  So for now I only
handle secret keys using the scheme derived above.

This has shown some promising results.  I had Pallab test the webserver with my
patch, and he reported the following:

              no-reuse  reuse
3.3.4:             508  1444
3.3.5beta:         530  1715
3.3.5beta + patch: 539  1912

	Ian McGreer Reporter
	Comment 1 • 22 years ago

This patch is for the 3.3 branch, because the performance framework we're using
is set up for that.  It applies fairly cleanly to the tip, but some things have
moved around.  I will produce a patch for the tip later.

I'm calling a rough draft because I still have some work to do, but it stands
up to the stress conditions of performance testing, so I think it is close. 
Here are some notes:

1) a new file pkcs11obj.c is created, to handle the object-specific attribute
handling.  Each object type inherits attributes as described in the spec.  All
share the commonObjectAttrsStr.

2) some attributes are marked RO, others RW.  The difference is which are
allowed to be modified after creation and which aren't, according to the spec.

3) RW attributes in general need lock protection, so that two threads cannot
attempt to modify them at the same time.  This is provided.  However, if an
object is not modifiable (CKA_MODIFIABLE == CK_FALSE), then all the RW attribs
are RO.  This means the lock operation can be skipped.	The patch implements
this, but NSS/SSL does not set CKA_MODIFIABLE in templates, and it is true by
default.  It may be worthwhile to see if there is a performance benefit to
creating all SSL keys as non-modifiable objects.

4) there needs to be a distinction between attributes not provided in a
template and defaults.	I added a special boolean value, ck_BOOL_NOT_SET, which
serves as a placeholder until object creation is complete, at which point it is
overriden by default values.

	Ian McGreer Reporter
	Comment 2 • 22 years ago

I forgot to mention, I provided the "rough draft" patch because I'd like some
early feedback on this approach ;).

	Robert Relyea
	Comment 3 • 22 years ago

You certainly started the hard way, persistant objects in NSS 3.4 are already
dealt with in the way you are describing. It was handled by hooking at the
set/clear attribute calls (so there as less code perturbing).

One global comment, I see lots of:

if (xxx->attrib) {
    value = getValue(xxx);
} else {
    attr = GetAttribute(xxx, CKA_VALUE);
    value = attr->ulValue;
    value.len = attr->len;
}

It would be cleaner to colapse the if (xxx->attrib) test into the getValue function.

bob

	Ian McGreer Reporter
	Comment 4 • 22 years ago

Well, yes and no... I don't think 3.4+ really does what I'm proposing.  For
token objects, attributes are either extracted from the low-level structure (the
one used by the db code), or simply return a default that cannot be changed
(which is the case for most of the boolean attributes that aren't kept in the
low-level structures and aren't written to the db).  I think the strategy for
session objects the tip would be the same as the proposed patch, I'm not sure
how the token object code would've helped.

As for the forks all over the patch for choosing between the "old" and "new" way
of getting attributes, that's one of the things I need to clean up :)

	Ian McGreer Reporter
	Comment 5 • 22 years ago

To clarify, I perturbed the code a lot because given the choice between a
function like

pk11_getAttribute(object, CKA_PRIVATE);

and one like

pk11_getIsPrivate(object)

I preferred the latter, because the first always involves a switch statement. 
But I may have to concede that and stay with the former, because it makes the
patch so much larger and riskier (I started doing it that way before I realized
how much it was going to change things, and I'm still concerned I didn't catch
everything).

	Robert Relyea
	Comment 6 • 22 years ago

We still need the switch because there is code that that doesn't know which
thing to call for which attributes (FindObjects comes to mind).

I don't mind bypassing the switch in those places where we know what we are
looking for.

bob

	Ian McGreer Reporter
	Comment 7 • 22 years ago

Right, I implemented a GetAttributes equivalent that works on templates and
handles generic attributes. I also implemented a MatchAttributes function to
work with FindObjects.  I was trying to change places in the code where we ask
for specific attributes (usually CKA_TOKEN, CKA_PRIVATE, and CKA_VALUE).  But
there's going to have to be a tradeoff between that and not having a huge patch,
at least for now.