223289 - [cookies] optimize nsCookie

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

i have a couple of trivial enhancements for nsCookie in mind...

Comment 1

[[e-mail]]

Attachment: remove mIsDomain (checked in)

mIsDomain isn't necessary since we use a leading dot on mHost to specify a
domain cookie. so, just nuke it everywhere... (the move of the .Trim() call is
so we can be sure there is no leading dot on the host, if it's an IP address.
better safe than sorry.)

Comment 2

[[e-mail]]

Comment on attachment 133881 [details] [diff] [review]
remove mIsDomain (checked in)

just kick me if you're averse to r+sr... ;)

Comment 3

[Darin Fisher]

Comment on attachment 133881 [details] [diff] [review]
remove mIsDomain (checked in)

you got it!  r+sr=darin :)

Comment 4

[[e-mail]]

Comment on attachment 133881 [details] [diff] [review]
remove mIsDomain (checked in)

checked this one in on trunk.

Comment 5

[[e-mail]]

Attachment: provide nsCookie::Create()

halves the number of allocations when creating an nsCookie, from 2 to 1. this
stores the strings contiguously, in the same allocation block as the nsCookie
itself; and provides nsCookie::Create and ::Destroy methods to replace |new|
and |delete|.

this actually simplifies code, and saves 1.1k of codesize on gcc3.3.1. some
might consider it overoptimization though, so i'm indifferent as to whether we
do this :)

Comment 6

[[e-mail]]

Comment on attachment 135176 [details] [diff] [review]
provide nsCookie::Create()

caillon, i know you like |placement new| fu, what do you think?

Comment 7

[[e-mail]]

oh, more importantly, it fixes a bug where nsMemory::Alloc could fail in the
nsCookie constructor, and we wouldn't know about it. not that it matters any...

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 135176 [details] [diff] [review]
provide nsCookie::Create()

And why am I reviewing cookie code again?  :)

>@@ -141,7 +144,10 @@ nsCookie::GetExpires(PRUint64 *aExpires)
>   } else {
>     *aExpires = Expiry() > nsInt64(0) ? PRInt64(Expiry()) : 1;
>   }
>   return NS_OK;
> }
> 
>-NS_IMPL_ISUPPORTS2(nsCookie, nsICookie, nsICookie2)
>+// break out the NS_IMPL_ISUPPORTS macro, since we have our own deletion function

break up

>+NS_IMPL_ADDREF(nsCookie)
>+NS_IMPL_RELEASE_WITH_DESTROY(nsCookie, Destroy())
>+NS_IMPL_QUERY_INTERFACE2(nsCookie, nsICookie2, nsICookie)


> class nsCookie : public nsICookie2
> {
>-  // this is required because we use a bitfield refcount member.
>+  // break out the NS_DECL_ISUPPORTS macro, since we use a bitfield refcount member

break up.

>+    nsCookie   *mNext;
>+    const char *mName;
>+    const char *mValue;
>+    const char *mHost;
>+    const char *mPath;
>+    const char *mEnd;
>+    nsInt64     mExpiry;
>+    nsInt64     mLastAccessed;
>+    PRUint32    mRefCnt    : 16;

Just use a PRUint16, man :)
Also, can't you use 4 bits each for the following fields?  Or maybe 8, 2, 3, 3
or something.  I think they may get handled better by some compilers, or at
least purify.

>+    PRUint32    mIsSession : 1;
>+    PRUint32    mIsSecure  : 1;
>+    PRUint32    mStatus    : 3;
>+    PRUint32    mPolicy    : 3;
> };
> 
> #endif // nsCookie_h__

>@@ -1043,22 +1043,22 @@ nsCookieService::Read()

>     // create a new nsCookie and assign the data.
>     newCookie =
>-      new nsCookie(Substring(buffer, nameIndex, cookieIndex - nameIndex - 1),
>-                   Substring(buffer, cookieIndex, buffer.Length() - cookieIndex),
>-                   host,
>-                   Substring(buffer, pathIndex, secureIndex - pathIndex - 1),
>-                   nsInt64(expires),
>-                   lastAccessedCounter,
>-                   PR_FALSE,
>-                   Substring(buffer, secureIndex, expiresIndex - secureIndex - 1).Equals(kTrue),
>-                   nsICookie::STATUS_UNKNOWN,
>-                   nsICookie::POLICY_UNKNOWN);
>+      nsCookie::Create(Substring(buffer, nameIndex, cookieIndex - nameIndex - 1),
>+                       Substring(buffer, cookieIndex, buffer.Length() - cookieIndex),
>+                       host,
>+                       Substring(buffer, pathIndex, secureIndex - pathIndex - 1),
>+                       nsInt64(expires),
>+                       lastAccessedCounter,
>+                       PR_FALSE,
>+                       Substring(buffer, secureIndex, expiresIndex - secureIndex - 1).Equals(kTrue),
>+                       nsICookie::STATUS_UNKNOWN,
>+                       nsICookie::POLICY_UNKNOWN);
>     if (!newCookie) {
>       return NS_ERROR_OUT_OF_MEMORY;
>     }

Don't forget to clean up the delete call I added in this vicinity.

Comment 9

[Darin Fisher]

Comment on attachment 135176 [details] [diff] [review]
provide nsCookie::Create()

it appears that the buffer that will be written to by StrBlockCopy is given as
aDest1.  you should document this in the comment above StrBlockCopy.

suppose you use |operator new()| instead of malloc to allocate the buffer...
then couldn't your nsCookie::Destroy method just look like this instead:

nsCookie::Destroy()
{
  delete this;
}

i think that is cleaner than manually calling the destructor and then calling
free.  plus, you could just define Destroy in the header file so it will be
inlined at the callsite.  err.. then why bother with Destroy at all?  seems
like |operator delete| is what you want.  then you wouldn't need to break open
the NS_DECL/NS_IMPL macros either.

Comment 10

[[e-mail]]

Attachment: nsCookie::Create() v2

yup, that sounds better... provided calling |operator delete| on an nsCookie
that was casted from a |new char[N]| call works as expected. i can't think of a
reason why it wouldn't.

i left the bitfields as they were because i remember reading in a C++ bitfield
reference, that compilers generate bitfields from whole words. this isn't the
case with gcc, but might be for other compilers, and we don't want to bloat
nsCookie. in addition, moving to PRUint16/PRPackedBool etc didn't seem to help
codesize any.

Comment 11

[Darin Fisher]

Comment on attachment 135771 [details] [diff] [review]
nsCookie::Create() v2

>Index: netwerk/cookie/src/nsCookie.cpp

>+  char *place = new char[sizeof(nsCookie) + stringLength];

hmm... i think this should be:

    char *place = ::operator new(sizeof(nsCookie) + stringLength);

looks like malloc ;)

i worry about mixing "array new" with regular delete on an object
pointer.  but, if you use "::operator new" then you shouldn't have
to worry about such things.  (hint: we do this in other places.)

one thing to keep in mind with bitfields and especially smaller
than WORD length variables is that they will require masking to
access.  as you strive to reduce the size of the objects we malloc,
do not forget about the codesize cost of the masking inserted by
the compiler.  PRIntn is often better :)

sr=darin if you use "::operator new" or determine that "new char[]"
is okay.

Comment 12

[[e-mail]]

>but, if you use "::operator new" then you shouldn't have to worry about such
>things.

yeah, that was my worry... i didn't know about calling ::operator new directly.
that sounds much more sane though. i'll check in with that change.

>as you strive to reduce the size of the objects we malloc, do not forget about
>the codesize cost of the masking inserted by the compiler.

right... although (with gcc at least) referencing bytes doesn't take more code
than referencing words, which is understandable. having said that, i tested
codesize after changing the bitfields to use more byte-friendly types - it
didn't change net codesize at all. i haven't figured out why :/

Comment 13

[[e-mail]]

... and that's all i had in mind. fixed.

Comment 14

[Darin Fisher]

sounds good dwitte.. thx :)