The default bug view has changed. See this FAQ.

[jsdbg2] Eliminate non-held scripts

RESOLVED FIXED in Future

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks: 1 bug)

unspecified
Future
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
I think eval scripts could be stored in the heldScripts WeakMap without any problems.
Component: JavaScript Debugging/Profiling APIs → JavaScript Engine
Product: Core → Core
(Assignee)

Comment 1

6 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

6 years ago
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.
Assignee: nobody → jorendorff
(Assignee)

Comment 3

6 years ago
Created attachment 550895 [details] [diff] [review]
part 2 - Give function scripts a pointer to the Function object that keeps them gc-live
Attachment #550895 - Flags: review?(jimb)
(Assignee)

Updated

6 years ago
Attachment #550891 - Flags: review?(jimb)
(Assignee)

Comment 4

6 years ago
Created attachment 550896 [details] [diff] [review]
part 3 - Remove Debugger.Script.prototype.live
Attachment #550896 - Flags: review?(jimb)
(Assignee)

Comment 5

6 years ago
Created attachment 550897 [details] [diff] [review]
part 4 - Remove non-held script support from js::Debugger
Attachment #550897 - Flags: review?(jimb)
(Assignee)

Comment 6

6 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

6 years ago
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.
Attachment #550930 - Flags: review?(jimb)
(Assignee)

Updated

6 years ago
Blocks: 676281
(Assignee)

Updated

6 years ago
Blocks: 674164
(Assignee)

Comment 8

6 years ago
Created attachment 550994 [details] [diff] [review]
part 5 - Make wrapScript more like getScriptFrame
Attachment #550930 - Attachment is obsolete: true
Attachment #550930 - Flags: review?(jimb)
Attachment #550994 - Flags: review?(jimb)
(Assignee)

Comment 9

6 years ago
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
Attachment #550895 - Attachment is obsolete: true
Attachment #550895 - Flags: review?(jimb)
Attachment #551020 - Flags: review?(jimb)
(Assignee)

Comment 10

6 years ago
Created attachment 551048 [details] [diff] [review]
part 2 - Populate JSScript::object for function scripts

Fix a bug.
Attachment #551020 - Attachment is obsolete: true
Attachment #551020 - Flags: review?(jimb)
Attachment #551048 - Flags: review?(jimb)

Updated

6 years ago
Attachment #550891 - Attachment is patch: true
Attachment #550891 - Attachment mime type: text/x-patch → text/plain

Updated

6 years ago
Attachment #550891 - Flags: review?(jimb) → review+

Updated

6 years ago
Attachment #551048 - Flags: review?(jimb) → review+

Comment 11

6 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

6 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

6 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

6 years ago
(In reply to Jim Blandy :jimb from comment #12)
> an expressible value.

What is "an expressible value"?

Updated

6 years ago
Attachment #550896 - Flags: review?(jimb)

Updated

6 years ago
Attachment #550897 - Flags: review?(jimb)

Updated

6 years ago
Attachment #550994 - Flags: review?(jimb)

Comment 15

5 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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.