Closed
Bug 665167
Opened 13 years ago
Closed 13 years ago
[jsdbg2] Eliminate non-held scripts
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: jorendorff, Assigned: jorendorff)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 3 obsolete files)
35.30 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
13.57 KB,
patch
|
Details | Diff | Splinter Review | |
27.06 KB,
patch
|
Details | Diff | Splinter Review | |
5.94 KB,
patch
|
Details | Diff | Splinter Review | |
4.09 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
I think eval scripts could be stored in the heldScripts WeakMap without any problems.
Updated•13 years ago
|
Component: JavaScript Debugging/Profiling APIs → JavaScript Engine
Assignee | ||
Comment 1•13 years ago
|
||
Slightly changing the meaning of this bug. Patches coming.
Summary: jsdbg2: consider merging js::Debug::{heldScripts,evalScripts} → [jsdbg2] Eliminate non-held scripts
Assignee | ||
Comment 2•13 years ago
|
||
This eliminates TCF_NEED_SCRIPT_OBJECT; all non-function scripts need a script object now. JSScript::u.nextToGC is no longer an accurate name, so rename it to JSScript::evalCacheChain. It can no longer be in a union with JSScript::u.object, since JSScript::object is now populated for eval scripts; so make them separate fields. Eliminate code to eagerly destroy scripts outside of GC. -65 lines of code net. Tests pass.
Assignee: nobody → jorendorff
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #550895 -
Flags: review?(jimb)
Assignee | ||
Updated•13 years ago
|
Attachment #550891 -
Flags: review?(jimb)
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #550896 -
Flags: review?(jimb)
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #550897 -
Flags: review?(jimb)
Assignee | ||
Comment 6•13 years ago
|
||
Part 4 is -160 lines net. Changing things so that this js::Debugger method can exist, with this signature: JSObject *wrapScript(JSContext *cx, JSScript *script); is the point of this patch queue. That makes getAllScripts (bug 676281) possible.
Assignee | ||
Comment 7•13 years ago
|
||
As soon as I wrote that, I realized that the signature could be improved a little. The callers all want the result in a Value.
Attachment #550930 -
Flags: review?(jimb)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #550930 -
Attachment is obsolete: true
Attachment #550930 -
Flags: review?(jimb)
Attachment #550994 -
Flags: review?(jimb)
Assignee | ||
Comment 9•13 years ago
|
||
Whoops, found two more spots where function scripts are created. The total number is 4: - JSScript::NewScriptFromCG, compilation - js_XDRFunctionObject, deserialization - Function.prototype is custom-built in js_InitFunctionClass - js_CloneFunctionObject
Attachment #550895 -
Attachment is obsolete: true
Attachment #550895 -
Flags: review?(jimb)
Attachment #551020 -
Flags: review?(jimb)
Assignee | ||
Comment 10•13 years ago
|
||
Fix a bug.
Attachment #551020 -
Attachment is obsolete: true
Attachment #551020 -
Flags: review?(jimb)
Attachment #551048 -
Flags: review?(jimb)
Updated•13 years ago
|
Attachment #550891 -
Attachment is patch: true
Attachment #550891 -
Attachment mime type: text/x-patch → text/plain
Updated•13 years ago
|
Attachment #550891 -
Flags: review?(jimb) → review+
Updated•13 years ago
|
Attachment #551048 -
Flags: review?(jimb) → review+
Comment 11•13 years ago
|
||
Jason - can you test the patches against TDHTML on try server? I suspect that they would show the same regression as I observe in bug 674251. For the try run I use: try: -b o -p all -u none -t nochrome
Comment 12•13 years ago
|
||
Do you still need a review on this, or has it been made obsolete by JSScript becoming an expressible value.
Target Milestone: --- → mozilla8
Version: Other Branch → 9 Branch
Comment 13•13 years ago
|
||
I had no intention of changing the Version and Target Milestone in the previous comment; I don't know what happened.
Target Milestone: mozilla8 → Future
Version: 9 Branch → unspecified
Comment 14•13 years ago
|
||
(In reply to Jim Blandy :jimb from comment #12) > an expressible value. What is "an expressible value"?
Updated•13 years ago
|
Attachment #550896 -
Flags: review?(jimb)
Updated•13 years ago
|
Attachment #550897 -
Flags: review?(jimb)
Updated•13 years ago
|
Attachment #550994 -
Flags: review?(jimb)
Comment 15•13 years ago
|
||
The current sources have only a single table of scripts, managing both eval scripts and other sorts of scripts, so this bug is resolved.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•