Closed Bug 1139473 Opened 7 years ago Closed 7 years ago

File metadata for the js/src subdirectory

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

Details

Attachments

(1 file, 1 obsolete file)

Following [0], let's start to file a few directories in the js/ subdir. Massive feedback requests incoming...

[0] https://mail.mozilla.org/pipermail/firefox-dev/2015-March/002760.html
Attached patch metadata.patch (obsolete) — Splinter Review
So I am starting with these few modifications, but if somebody who knows all the different categories more than I do and they want to take the patch and continue working on it, I'm cool with that :)

gps, can we define directories that are above pwd? For instance, I've used |with Files('../public')|, as the ../public directory doesn't have its own mozbuild file and is used by js, but the doc doesn't say if it's permitted or not.

Terrence, are there other specific files/directories that you would put in the GC component?

Waldo/Till, same question for the standard lib / Intl component?

Jan, same question for the jit.

Everybody, feel free to chime in :)
Attachment #8572673 - Flags: feedback?(till)
Attachment #8572673 - Flags: feedback?(terrence)
Attachment #8572673 - Flags: feedback?(jwalden+bmo)
Attachment #8572673 - Flags: feedback?(jdemooij)
Attachment #8572673 - Flags: feedback?(gps)
Comment on attachment 8572673 [details] [diff] [review]
metadata.patch

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

Looks good but some comments below.

::: js/src/moz.build
@@ +18,5 @@
> +    BUG_COMPONENT = ('Core', 'js-ctypes')
> +with Files('gc/**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript: GC')
> +with Files('irregexp/**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine: JIT')

"JavaScript Engine" is probably slightly better here. irregexp has a JIT but it also has an interpreter, regexp parser/optimizer, etc.

@@ +22,5 @@
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine: JIT')
> +with Files('jit/**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine: JIT')
> +with Files('jit-tests/**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine: JIT')

I think we should use "JavaScript Engine" here too. It's called *jit*-tests and many tests are jit-related, but there are also a ton of other tests and it's not inherently JIT-only.
Attachment #8572673 - Flags: feedback?(jdemooij) → feedback+
Comment on attachment 8572673 [details] [diff] [review]
metadata.patch

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

::: js/src/moz.build
@@ +16,5 @@
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine: Standard Library')
> +with Files('ctypes/**'):
> +    BUG_COMPONENT = ('Core', 'js-ctypes')
> +with Files('gc/**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript: GC')

Please also add devtools/rootAnalysis, devtools/gc-ubench, and devtools/gctrace.
Attachment #8572673 - Flags: feedback?(terrence) → feedback+
Comment on attachment 8572673 [details] [diff] [review]
metadata.patch

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

::: js/src/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# Directory metadata
> +with Files('../public/**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine')

There are some GC-specific files in js/public, so until Files() accepts multiple dirs, please add

for header in ('GCAPI.h', 'HeapAPI.h', 'RootingAPI.h', 'SliceBudget.h', 'TracingAPI.h', WeakMapPtr.h'):
    with Files('../public/' + header):
        BUG_COMPONENT = ('Core', 'JavaScript: GC')

@@ +7,5 @@
> +# Directory metadata
> +with Files('../public/**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine')
> +with Files('**/**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine')

Couldn't this just be Files('**')?

@@ +26,5 @@
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine: JIT')
> +
> +# File-specific metadata
> +with Files('jsgc**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript: GC')

Why ** ? I would expect just jsgc*
Thanks all for the first feedbacks! I've taken your comments into account in my local copy.
Comment on attachment 8572673 [details] [diff] [review]
metadata.patch

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

LGTM.

Nit: is a pattern ends with '*', there is no difference between '*' and '**'. But no harm no foul on '**'.

::: js/src/moz.build
@@ +4,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# Directory metadata
> +with Files('../public/**'):

I'm not sure how I feel about the leading '..'. If it just works, great. But part of me thinks there should be a moz.build in js/ or js/public/.

@@ +7,5 @@
> +# Directory metadata
> +with Files('../public/**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine')
> +with Files('**/**'):
> +    BUG_COMPONENT = ('Core', 'JavaScript Engine')

This can actually just be '*'.

See https://gecko.readthedocs.org/en/latest/build/buildsystem/mozbuild-symbols.html#mozbuild-subcontext-files
Attachment #8572673 - Flags: feedback?(gps) → feedback+
Comment on attachment 8572673 [details] [diff] [review]
metadata.patch

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

There are also a few files in js/src that should probably be marked as "JavaScript Engine: Standard Library":
- jsarray.* (well, maybe, I'm not 100% sure.)
- jsbool* (again, not entirely sure.)
- jsdate.*
- jsnum.* (?)
- json.*
- jsreflect.* (though it's not standardized.)
- jsstr.* (?)
Attachment #8572673 - Flags: feedback?(till) → feedback+
Comment on attachment 8572673 [details] [diff] [review]
metadata.patch

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

If we're going to do this -- and it looks cool, we should do it -- we should add the additional components we obviously need.

Core :: JavaScript Engine: Parser and Bytecode (frontend/*, vm/make_opcode_doc.py, vm/Opcodes.h, jsopcode*, jskwgen.cpp)
Core :: JavaScript Engine: Debugging API (vm/Debugger*, vm/UbiNode.cpp, ../public/Debug.h, ../public/Profiling*, ../public/UbiNode*)

It's been in the back of my mind to file bugs to have these additional components for a long while now.  I probably even have some stale Firefox tabs open to the enter_bug.cgi page to file them.  (Awesomebar isn't finding them right now, tho -- probably because [gulp] I haven't loaded them within the depth of my cached browsing history.)  I'm pretty sure jimb okayed the latter at the time, on IRC.

../public/TrackedOptimizationInfo.h should probably be JIT, right?

Do we want profiling support included in a Debugging API component, or do we want a separate Profiler component?  Those files are ../public/Profiling*, builtin/Profilers.*, vm/SPSProfiler.*.

It might be worth having a JavaScript Engine: Shell component for shell/*, builtin/TestingFunctions.*.
Attachment #8572673 - Flags: feedback?(jwalden+bmo) → feedback+
Attached patch metadata.patchSplinter Review
Asking review to jorendorff, as module owner; gps, as he proposed the initiative.

Also, letting module / submodule owners decide about creating other components. They all seem to make sense, but wouldn't having too many submodules make triage harder?
Attachment #8572673 - Attachment is obsolete: true
Attachment #8573867 - Flags: review?(jorendorff)
Attachment #8573867 - Flags: review?(gps)
Attachment #8573867 - Flags: review?(jorendorff) → review+
Comment on attachment 8573867 [details] [diff] [review]
metadata.patch

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

You don't need my review on the final version.

::: js/src/moz.build
@@ +8,5 @@
> +COMPONENT_ENGINE = ('Core', 'JavaScript Engine')
> +COMPONENT_GC     = ('Core', 'JavaScript: GC')
> +COMPONENT_INTL   = ('Core', 'JavaScript: Internationalization API')
> +COMPONENT_JIT    = ('Core', 'JavaScript Engine: JIT')
> +COMPONENT_STL    = ('Core', 'JavaScript: Standard Library')

This should break moz.build processing since UPPERCASE variables are all reserved and these aren't known variables.
Attachment #8573867 - Flags: review?(gps) → feedback+
Assuming the red failures aren't due to this but are intermittent, attemptively pushing...

https://hg.mozilla.org/integration/mozilla-inbound/rev/52b8c07bfccd
Flags: needinfo?(benj)
https://hg.mozilla.org/mozilla-central/rev/52b8c07bfccd
Assignee: nobody → benj
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.