779233 - move filename to ScriptSource

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Benjamin Peterson]

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?

Comment 1

[Nicholas Nethercote [inactive]]

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.

Comment 2

[:Benjamin Peterson]

Attachment: move filename to ScriptSource

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.

Comment 3

[:Benjamin Peterson]

Attachment: count filename memory as script source memory

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.

Comment 4

[Nicholas Nethercote [inactive]]

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.

Comment 5

[Nicholas Nethercote [inactive]]

> 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 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 724161 [details] [diff] [review]
move filename to ScriptSource

Thanks!

Comment 7

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c51d394e31f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4945329805

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c51d394e31f3
https://hg.mozilla.org/mozilla-central/rev/2b4945329805