Add function names to module factories in DEV

[gaearon]

This adds path-based function names to the generated module factories in development.

__d(function src_some_folder_path_MyComponent_js__module_factory__() {
  // ...
})

They're only emitted when minify is false and dev is true.

Motivation

Profiling module initialization is a common need. The current set of tooling React Native provides for it is arguably abysmal.

The primary tool suggested by React Native documentation is Systrace which literally does not exist anymore. "Profiling with Hermes" suggests using sampling profiler which seems to barely work — I wasn't able to capture any information with it.

If you try to use Hermes Chrome DevTools integration and press "Record" in the Performance tab, it hangs.

I almost gave up at that point, but @brentvatne kindly suggested that there is a workaround — I need to Shift+Cmd+P to get a different "JS Profiler" Chrome DevTools tab to show up, and that one actually works.

Even that approach is a little bit broken but it's the closest to what I want.

Unfortunately, even that tool does not let me see which modules I'm spending time initializing:

As you can see, module factories have blank function names.

This is not the case with web-based tooling. For example, webpack emits module factories as a dictionary like { "./src/some/module.js": function() {}, ... } so the stacks are useful. Since we don't want to change the format entirely, and configuring fn.name dynamically at runtime does not make it show up in the trace, I opted for a minimal possible change — to actually emit the module names in development.

As a result, I can see where I'm spending time during module initialization:

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6151e7e) 83.09% compared to head (eaf9dd1) 83.10%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1130   +/-   ##
=======================================
  Coverage   83.09%   83.10%           
=======================================
  Files         206      206           
  Lines       10547    10552    +5     
  Branches     2617     2619    +2     
=======================================
+ Hits         8764     8769    +5     
  Misses       1783     1783           

Files	Coverage Δ
packages/metro-transform-worker/src/index.js	87.19% <100.00%> (+0.15%)	⬆️
...ges/metro/src/ModuleGraph/worker/JsFileWrapping.js	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[facebook-github-bot]

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[motiz88]

My main concern here (other than the obvious: we should improve the profiling & debugging tools so that this isn't needed 😅) is around the portability and cache consistency of using this filename property.

I'm not sure off the top of my head whether this is (1) an absolute or relative path, (2) a platform-dependent or normalised path. Could you confirm?

• If the path is absolute, then transformer cache artifacts will be sensitive to the project's root directory, which is incorrect. (Cache artifacts should be compatible across machines, including e.g. checkouts at different base locations)
• If the path is absolute and platform-dependent, then it would begin with C:\foo on Windows but /foo on POSIX, which again makes the cache non-portable.

[gaearon]

This is a relative path in my testing — see the screenshot (e.g. node_modules/blabla/...).

I'm not sure if it's platform-dependent or not but we replace all non-valid JS ident characters with _ so a path with / and \ would result in the same identifier.

[gaearon]

Note that we already emit a path into the DEV artifacts here (the module's "verbose name") but it happens at a different stage so I'm not entirely sure if they're constructed in the same way or come from different sources.

[facebook-github-bot]

@robhogan merged this pull request in 2f62858.

[facebook-github-bot]

This pull request has been reverted by 92340b3.

[robhogan]

Urgh - this caused native crashes across multiple apps in debug builds post-land, still connecting the dots but there are runtime mem allocation failures coming from Hermes - first few stack frames, FYI:

SIGABRT:libxplat_hermes_API_HermesAPIAndroid.so:./xplat/hermes/external/llvh/include/llvh/Support/MemAlloc.h:llvh::SmallVectorBase::grow_pod(void*, unsigned int, unsigned int)

hermes::irgen::ESTreeIRGen::serializeScope(hermes::ScopeDesc*, bool)
hermes::irgen::ESTreeIRGen::setupLazyScope(hermes::ESTree::FunctionLikeNode*, hermes::Function*, hermes::ESTree::BlockStatementNode*)
hermes::irgen::ESTreeIRGen::genES5Function(hermes::Identifier, hermes::Variable*, hermes::ESTree::FunctionLikeNode*, bool)

[gaearon]

Is there any chance that some (relative) paths are still incredibly long, or that Hermes has a limit on function names that isn't present in other JS engines? I was thinking of adding .slice(0, 50) or something.

[robhogan]

Is there any chance that some (relative) paths are still incredibly long, or that Hermes has a limit on function names that isn't present in other JS engines? I was thinking of adding .slice(0, 50) or something.

String length is a factor, but probably not the determining one. We saw total RAM on Facebook Android (dev builds) go from 1.6GB to 3.8GB with this change, so way more than just the sum of those strings, even though the paths are pretty long.

@tmikov has been working on it and identified a quadratic growth situation where scopes were repeatedly re-serialised during lazy evaluation, but the fix for that didn't seem to be enough on its own.

Worst case, we could enable this behind a serializer config flag in Metro and document the memory use, but the Hermes folks were actively looking into it last I heard so they may want a bit more time.

[tmikov]

There is no limit, Hermes is just not optimized for running from source - this problem doesn't exist when running from bytecode. Running from source with lazy compilation is, in a sense, contrary to Hermes's mission of being an optimizing AOT compiler. We basically bolted lazy compilation on top of the AOT compiler and had to use a lot of hacks, because it simply wasn't designed for that. It is a miracle it has worked as well as it has...

We will support this properly in Static Hermes (at tremendous complexity cost), but for now we are trying to figure out exactly what is consuming the memory in "regular" Hermes. There is something quadratic. (We already fixed one quadratic case, but sadly it made no difference.)