1139473 - File metadata for the js/src subdirectory

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: metadata.patch

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 :)

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Terrence Cole [:terrence]]

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.

Comment 4

[Steve Fink [:sfink] [:s:]]

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*

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks all for the first feedbacks! I've taken your comments into account in my local copy.

Comment 6

[Gregory Szorc [:gps]]

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

Comment 7

[Till Schneidereit [:till]]

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.* (?)

Comment 8

[Jeff Walden [:Waldo]]

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.*.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: metadata.patch

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?

Comment 10

[Gregory Szorc [:gps]]

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.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4910b38cd44c

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Assuming the red failures aren't due to this but are intermittent, attemptively pushing...

https://hg.mozilla.org/integration/mozilla-inbound/rev/52b8c07bfccd

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52b8c07bfccd