Closed
Bug 381779
Opened 18 years ago
Closed 18 years ago
JS decompiler uses more memory for sprint stack than necessary
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: gavin.reaney, Assigned: gavin.reaney)
References
Details
Attachments
(2 files, 2 obsolete files)
2.32 KB,
text/plain
|
Details | |
3.99 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
There are a couple of cases where extra memory is being requested for the sprint stack when it isn't needed. These are:
(1) In PushOff, 3 bytes are always requested for PAREN_SLOP even when there is enough free space at sp->offset.
(2) In SprintString, the entire string length is being requested.
As far as I can tell, we only need to ensure that sp->size - sp->offset is large enough to hold the required number of bytes. If it is not, we should call SprintAlloc but request only the extra bytes needed.
For the attached test case, we request 1175 bytes for the sprint stack buffer, but only 247 bytes are needed. [Debug can be added to the end of js_DecompileCode to print out the final size of the sprint stack].
Assignee | ||
Comment 1•18 years ago
|
||
Fix the problem by adding extra checks before calling through to SprintAlloc.
An alternative approach would be to move the knowledge of sp->offset into SprintAlloc. I guess the original intent is that SprintAlloc shouldn't need to know about the meaning of offset, so I've not taken that approach.
Comment 2•18 years ago
|
||
I wouldn't mind SprintEnsureBuffer taking the number of bytes (not including the space for the NUL) that does the small amount of work currently open-coded. If that doesn't measure as worth it in terms of compiled code size, the current patch is ok. What do you think?
/be
Assignee | ||
Comment 3•18 years ago
|
||
Comment on attachment 265848 [details] [diff] [review]
Fix
Igor, are you available to review this change?
I noticed that you added the SprintString code so hopefully this is OK. Feel free to nominate another reviewer if you are busy.
This patch has been run through the decompiler unit tests in mozilla/js/tests as well as the test case in the bug report (comparing the decompiler output in patched and unpatched builds).
Attachment #265848 -
Flags: review?(igor)
Assignee | ||
Comment 4•18 years ago
|
||
(In reply to comment #2)
> I wouldn't mind SprintEnsureBuffer taking the number of bytes (not including
> the space for the NUL) that does the small amount of work currently open-coded.
> If that doesn't measure as worth it in terms of compiled code size, the current
> patch is ok. What do you think?
>
> /be
>
I think I'll change it as you suggest. It won't make a huge difference to code size, but it will help to prevent future cases of over allocation if new calls to SprintAlloc/SprintEnsureBuffer are added.
Assignee | ||
Updated•18 years ago
|
Attachment #265848 -
Attachment is obsolete: true
Attachment #265848 -
Flags: review?(igor)
Assignee | ||
Comment 5•18 years ago
|
||
Patch that uses the SprintEnsureBuffer approach.
There is a bug with the change to PushOff in the previous fix - no space for NUL was added (but it is added in this new patch).
One slight caveat regarding this patch: the changes here are more likely to expose any lurking bugs where sp->offset may have accidentally been increased beyond sp->size. For example, around line 3100 of jsopcode.c there is a call to AddParenSlop. I'm assuming that some earlier code has ensured that space was allocated for that.
Assignee | ||
Comment 6•18 years ago
|
||
Comment on attachment 265954 [details] [diff] [review]
Fix with SprintAlloc replaced by SprintEnsureBuffer
This is ready again. Brendan, feel free to comment or review if you prefer.
I've asked Igor to take a look at this since I'm on the lookout for another voucher :-)
Attachment #265954 -
Flags: review?(igor)
Comment 7•18 years ago
|
||
Comment on attachment 265954 [details] [diff] [review]
Fix with SprintAlloc replaced by SprintEnsureBuffer
>@@ -429,17 +433,17 @@ SprintString(Sprinter *sp, JSString *str
> ptrdiff_t offset;
>
> chars = JSSTRING_CHARS(str);
> length = JSSTRING_LENGTH(str);
> if (length == 0)
> return sp->offset;
>
> size = js_GetDeflatedStringLength(sp->context, chars, length);
>- if (size == (size_t)-1 || !SprintAlloc(sp, size + 1))
>+ if (!SprintEnsureBuffer(sp, size))
> return -1;
The check size == (size_t)-1 here indicates an error and must be kept as is to return from SprintString immediately with -1 and propagate the error to the caller.
Otherwise the patch is OK.
Attachment #265954 -
Flags: review?(igor) → review-
Assignee | ||
Comment 8•18 years ago
|
||
Thanks Igor. I removed that check unintentionally - now reinstated.
Also, I don't think it is necessary to add space for NUL after PAREN_SLOP, but doing so does no harm and keeps the code consistent.
Attachment #265954 -
Attachment is obsolete: true
Attachment #266037 -
Flags: review?(igor)
Comment 9•18 years ago
|
||
(In reply to comment #8)
> Also, I don't think it is necessary to add space for NUL after PAREN_SLOP, but
> doing so does no harm and keeps the code consistent.
Indeed, lets leave that optimization for another bug to make each patch to do one thing.
Updated•18 years ago
|
Attachment #266037 -
Flags: review?(igor) → review+
Comment 10•18 years ago
|
||
I will land the patch from comment 8 shortly.
Comment 11•18 years ago
|
||
I committed the patch from comment 8 to the trunk:
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v <-- jsopcode.c
new revision: 3.246; previous revision: 3.245
done
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•