Last Comment Bug 665167 - [jsdbg2] Eliminate non-held scripts
: [jsdbg2] Eliminate non-held scripts
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Future
Assigned To: Jason Orendorff [:jorendorff]
: jsd
Mentors:
Depends on:
Blocks: 674164 676281
  Show dependency treegraph
 
Reported: 2011-06-17 14:27 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-11-18 13:53 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - Give eval scripts a Script object (35.30 KB, patch)
2011-08-04 16:51 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Review
part 2 - Give function scripts a pointer to the Function object that keeps them gc-live (2.71 KB, patch)
2011-08-04 16:56 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
part 3 - Remove Debugger.Script.prototype.live (13.57 KB, patch)
2011-08-04 16:57 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
part 4 - Remove non-held script support from js::Debugger (27.06 KB, patch)
2011-08-04 16:59 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
part 5 - Make wrapScript more like getScriptFrame (5.85 KB, patch)
2011-08-04 19:29 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
part 5 - Make wrapScript more like getScriptFrame (5.94 KB, patch)
2011-08-05 03:41 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
part 2 - Populate JSScript::object for function scripts (4.09 KB, patch)
2011-08-05 06:06 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
part 2 - Populate JSScript::object for function scripts (4.09 KB, patch)
2011-08-05 08:49 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Review

Description Jason Orendorff [:jorendorff] 2011-06-17 14:27:54 PDT
I think eval scripts could be stored in the heldScripts WeakMap without any problems.
Comment 1 Jason Orendorff [:jorendorff] 2011-08-04 16:43:59 PDT
Slightly changing the meaning of this bug. Patches coming.
Comment 2 Jason Orendorff [:jorendorff] 2011-08-04 16:51:31 PDT
Created attachment 550891 [details] [diff] [review]
part 1 - Give eval scripts a Script object

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.
Comment 3 Jason Orendorff [:jorendorff] 2011-08-04 16:56:00 PDT
Created attachment 550895 [details] [diff] [review]
part 2 - Give function scripts a pointer to the Function object that keeps them gc-live
Comment 4 Jason Orendorff [:jorendorff] 2011-08-04 16:57:55 PDT
Created attachment 550896 [details] [diff] [review]
part 3 - Remove Debugger.Script.prototype.live
Comment 5 Jason Orendorff [:jorendorff] 2011-08-04 16:59:29 PDT
Created attachment 550897 [details] [diff] [review]
part 4 - Remove non-held script support from js::Debugger
Comment 6 Jason Orendorff [:jorendorff] 2011-08-04 17:01:56 PDT
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.
Comment 7 Jason Orendorff [:jorendorff] 2011-08-04 19:29:47 PDT
Created attachment 550930 [details] [diff] [review]
part 5 - Make wrapScript more like getScriptFrame

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.
Comment 8 Jason Orendorff [:jorendorff] 2011-08-05 03:41:07 PDT
Created attachment 550994 [details] [diff] [review]
part 5 - Make wrapScript more like getScriptFrame
Comment 9 Jason Orendorff [:jorendorff] 2011-08-05 06:06:08 PDT
Created attachment 551020 [details] [diff] [review]
part 2 - Populate JSScript::object for function scripts

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
Comment 10 Jason Orendorff [:jorendorff] 2011-08-05 08:49:33 PDT
Created attachment 551048 [details] [diff] [review]
part 2 - Populate JSScript::object for function scripts

Fix a bug.
Comment 11 Igor Bukanov 2011-08-29 08:27:08 PDT
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 Jim Blandy :jimb 2011-09-22 13:59:47 PDT
Do you still need a review on this, or has it been made obsolete by JSScript becoming an expressible value.
Comment 13 Jim Blandy :jimb 2011-09-22 14:08:23 PDT
I had no intention of changing the Version and Target Milestone in the previous comment; I don't know what happened.
Comment 14 Igor Bukanov 2011-09-22 14:28:39 PDT
(In reply to Jim Blandy :jimb from comment #12)
> an expressible value.

What is "an expressible value"?
Comment 15 Jim Blandy :jimb 2011-11-18 13:53:36 PST
The current sources have only a single table of scripts, managing both eval scripts and other sorts of scripts, so this bug is resolved.

Note You need to log in before you can comment on or make changes to this bug.