Closed
Bug 61482
Opened 24 years ago
Closed 24 years ago
JS shared getters returning nonces entrain too much garbage
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla0.8
People
(Reporter: brendan, Assigned: brendan)
Details
(Keywords: js1.5)
Attachments
(4 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
A JS property getter for a dynamic query-response object, or similar nonce,
should not store the strong ref to that object in the property's slot. Doing so
tends to pile up garbage from the last-got value, which can be quite large.
Patch coming up.
/be
Assignee | ||
Updated•24 years ago
|
Assignee | ||
Comment 1•24 years ago
|
||
Assignee | ||
Comment 2•24 years ago
|
||
Idea: don't add a JSPROP_NOSLOT pseudo-attribute (and related JSCLASS_NOSLOTS
class flag); do define the new-in-js1.5 JSPROP_SHARED attribute to mean "don't
allocate a slot" (so the property's slot member is SPROP_INVALID_SLOT). Shared
properties currently are __proto__ and __parent__. These properties' getters
and setter magically update the 0th (proto) and 1st (parent) slots in the object
being operated upon, even though the properties live in Object.prototype (and
never get cloned on set into derived objects).
The properties for __proto__ and __parent__ in Object.prototype do not need
slots in Object.prototype. In fact those slots currently can entrain garbage.
Revised patch coming up.
/be
Assignee | ||
Comment 3•24 years ago
|
||
Assignee | ||
Comment 4•24 years ago
|
||
js1.5 priority issue, for external contributors.
/be
Priority: P3 → P1
Assignee | ||
Comment 5•24 years ago
|
||
Comment 6•24 years ago
|
||
Has this fix gone in with that for 54743?
Assignee | ||
Comment 7•24 years ago
|
||
Nope, still looking for r= and sr=. Here comes another patch that uses
JSPROP_SHARED more, for every READONLY property predefined on a class prototype
native object whose getter recomputes the property value and does not rely on a
slot to "cache" old values.
/be
Assignee | ||
Comment 8•24 years ago
|
||
Assignee | ||
Updated•24 years ago
|
Keywords: mozilla0.8
Comment 10•24 years ago
|
||
Not so related to this bug, but - does JS_BUG_AUTO_INDEX_SLOTS need an #else?
Add a backstop against SPROP_INVALID_SLOT in js_FreeSlot?
jsobj.c:163,257 aren't guarded with SPROP_INVALID_SLOT check. (163 is getter
for Object; is that lowlevel enough not to matter?)
Would it make sense to fold the SPROP_INVALID_SLOT check into
[LOCKED_]OBJ_GET|SET_SLOT? (With checks against PARENT, PROTO, PRIVATE?)
Assignee | ||
Comment 11•24 years ago
|
||
>Not so related to this bug, but - does JS_BUG_AUTO_INDEX_SLOTS need an #else?
No, believe it or not -- that #if block sets foo[0] when foo.first is set, but
the code after the then part is the property cache fill for foo.first, so it
should not be the else part.
>Add a backstop against SPROP_INVALID_SLOT in js_FreeSlot?
Already there -- see OBJ_CHECK_SLOT.
>jsobj.c:163,257 aren't guarded with SPROP_INVALID_SLOT check. (163 is getter
>for Object; is that lowlevel enough not to matter?)
Those are for proto or parent slots, which all objects have. The slot numbers
come from the first two object_props entries' tinyid initializers.
>Would it make sense to fold the SPROP_INVALID_SLOT check into
>[LOCKED_]OBJ_GET|SET_SLOT? (With checks against PARENT, PROTO, PRIVATE?)
No, that would be an unnecessary performance penalty, a redundant check in the
case of the well-known slots. The layering works so that where there is an
sprop parameter (which may have lived beyond a JS_ClearScope due to jsdbgapi.h
usage, or which may be JSPROP_SHARED, hence SPROP_INVALID_SLOT), the check must
be made. Where there is no such sprop parameter, only a slot param, the slot
must be valid.
/be
Comment 12•24 years ago
|
||
sr=jband
Comment 13•24 years ago
|
||
r=mccabe. Thanks for the answers!
Assignee | ||
Comment 14•24 years ago
|
||
Fix checked in, thanks.
/be
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Updated•24 years ago
|
Keywords: mozilla0.8
You need to log in
before you can comment on or make changes to this bug.
Description
•