283761 - selfserv use of memset is inefficient

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

As part of the handleConnection function, selfserv performs a large memset (of
10KB) to zero a stack buffer, before invoking PR_Read. This is not really
necessary. All that's needed is to NULL-terminate the buffer after a read.
This particular memset is showing up as the most frequent CPU stack in
performance profiles, thus it somewhat invalidates any performance made with
selfserv.

Comment 1

[Julien Pierre]

Attachment: remove memset and NULL terminate

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 175622 [details] [diff] [review]
remove memset and NULL terminate

r=nelson
I believe the code is as correct with this patch as without it.
Here is a suggestion for an additional improvement to the existing code.

>@@ -885,21 +884,23 @@
>     }
> 
>     while (1) {

Suggest you change that to 
      while (bufRem > 0) {

> 	newln = 0;
> 	reqLen     = 0;
>-	rv = PR_Read(ssl_sock, pBuf, bufRem);
>+	rv = PR_Read(ssl_sock, pBuf, bufRem - 1);

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Jullien checked in the above patch.

Comment 4

[Wan-Teh Chang]

Comment on attachment 175622 [details] [diff] [review]
remove memset and NULL terminate

This patch is correct.	There is a subtle
difference from the old code because the
old code does not null terminate 'buf' if
the PR_Read call reads in 'bufRem' bytes
of data.  So the new code is more correct.

I agree that the sole purpose of the memset
call was to ensure that data in 'buf' was
null terminated, and our code depends on
that, because it passes 'buf' to string
functions such as PORT_Strstr and strncmp.

Comment 5

[Wan-Teh Chang]

Re: Nelson's suggested change to the condition
for the while loop

In the new code, bufRem is always >= 1.  So

    while (bufRem > 0) {

is the same as

    while (1) {

The loop will terminate when bufRem becomes 1.
At that time, we will pass 0 as the third
argument to PR_Read, and PR_Read will return
either 0 or -1, both of which cause us to leave
the while loop.

We should avoid passing 0 as the third argument
to PR_Read, but I am not sure if we can simply
say

    while (bufRem > 1) {

because the proper action may be "goto cleanup".