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)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gavin.reaney, Assigned: gavin.reaney)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file Test case
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].
Attached patch Fix (obsolete) — Splinter Review
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.
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
Blocks: 366230
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)
(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.
Attachment #265848 - Attachment is obsolete: true
Attachment #265848 - Flags: review?(igor)
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.
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 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-
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)
(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.
Attachment #266037 - Flags: review?(igor) → review+
I will land the patch from comment 8 shortly.
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
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: