Closed Bug 779233 Opened 7 years ago Closed 7 years ago

move filename to ScriptSource

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: Benjamin, Assigned: Benjamin)

References

Details

(Whiteboard: [js:t])

Attachments

(2 files)

Since scripts in a compilation unit usually share the same filename, we could save a word on JSScript by putting the filename on ScriptSource. I say "usually" because the //@line directive can change the filename. I don't know how important it is for scripts to be able to pretend they're from different files. Will the whole thing be superseded by source maps? Maybe we could just take the first //@line?
Whiteboard: [js:t]
The semantics of //@line are unclear, and the implementation is really hacky, working just well enough for the particular uses we have in Firefox.  I think it currently only works if you change the filename once in a file, but I'm not certain.
Depends on: 842438
Bill, I remember you telling me you wanted to kill the script filename table stuff. Are you okay to review, too? Most of patch is just making JSScript::filename into a method.
Attachment #724161 - Flags: review?(wmccloskey)
With the previous patch, script filenames no longer live in a separate hashtable. Is it okay if I just count them as part of the script source now? I never noticed that they ever amounted to much.
Attachment #724162 - Flags: review?(n.nethercote)
Comment on attachment 724162 [details] [diff] [review]
count filename memory as script source memory

Review of attachment 724162 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1926,5 @@
>                    "the runtime.");
>  
>      RREPORT_BYTES(rtPath + NS_LITERAL_CSTRING("runtime/script-sources"),
>                    nsIMemoryReporter::KIND_HEAP, rtStats.runtime.scriptSources,
>                    "Memory use for storing JavaScript source code.");

Please add "and script filenames" to the description.
Attachment #724162 - Flags: review?(n.nethercote) → review+
> Most of patch is just making JSScript::filename into a method.

It makes life easier for reviewers if you split out the big mechanical change into a separate patch.
Comment on attachment 724161 [details] [diff] [review]
move filename to ScriptSource

Thanks!
Attachment #724161 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/c51d394e31f3
https://hg.mozilla.org/mozilla-central/rev/2b4945329805
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 850928
You need to log in before you can comment on or make changes to this bug.