Closed
Bug 283761
Opened 19 years ago
Closed 19 years ago
selfserv use of memset is inefficient
Categories
(NSS :: Tools, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.10
People
(Reporter: julien.pierre, Assigned: julien.pierre)
Details
Attachments
(1 file)
1.14 KB,
patch
|
nelson
:
review+
wtc
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•19 years ago
|
Priority: -- → P2
Target Milestone: --- → 3.10
Assignee | ||
Comment 1•19 years ago
|
||
Attachment #175622 -
Flags: review?(nelson)
Comment 2•19 years ago
|
||
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);
Attachment #175622 -
Flags: review?(nelson) → review+
Comment 3•19 years ago
|
||
Jullien checked in the above patch.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 4•19 years ago
|
||
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.
Attachment #175622 -
Flags: superreview+
Comment 5•19 years ago
|
||
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".
You need to log in
before you can comment on or make changes to this bug.
Description
•