Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve hook names performance with extended source maps #21782

Closed
bvaughn opened this issue Jul 1, 2021 · 21 comments
Closed

Improve hook names performance with extended source maps #21782

bvaughn opened this issue Jul 1, 2021 · 21 comments

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Jul 1, 2021

#21641 adds the ability for DevTools to display hook "names". The way this works is:

  1. Run the component function to detect where all the hook calls are made (by inspecting the call stack).
  2. Load source files.
  3. Look for source maps and load them (if they exist).
  4. Parse source maps.
  5. Convert line/column number in compiled code to original source code.
  6. Parse original source code into AST (this is expensive).
  7. Extract names.

What if we had a custom transform that inferred these names during compilation and inserted metadata into the source map itself? Then DevTools could bail after step 4 and skip the expensive AST parsing step and the heuristics for inferring hook names.

It could still fall back to the more expensive step if this metadata wasn't available, but this would enable the feature to better support large apps where parsing the AST is prohibitively expensive.

See this comment below for a detailed explanation of how this might be implemented: #21782 (comment)

@houssemchebeb
Copy link
Contributor

houssemchebeb commented Jul 1, 2021

Hi @bvaughn, Can I give this a try?

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 1, 2021

@houssemchebeb Awesome! Have you worked with source map extensions before? This will be an un-guided task (since I don't have any experience working with this stuff yet).

We'd be happy to have your help on this! Just checking to make sure that the task is a good fit. :)

@houssemchebeb

This comment has been minimized.

@bvaughn

This comment has been minimized.

@houssemchebeb

This comment has been minimized.

@motiz88
Copy link
Contributor

motiz88 commented Jul 1, 2021

I'd like to propose an approach and offer some pointers. This is based on a conversation I had earlier with @bvaughn and some work I did a while ago on source maps in React Native and Metro.

Context: Function maps in Metro

For React Native, we wanted to reliably "unminify" function names in stack traces, and even show inferred names for anonymous functions, without re-parsing the original code and without increasing the size of our production bundles. We came up with a way to store extra information in a bundle's source map and used it to store function maps generated from all the source files at compile time.

This turns out to be similar to the problem of inferring hook names at build time, so let me first describe our solution for function names in detail.

Step 1: AST → function map

A function map is an ordered list of (line, column, name) tuples describing the local function name at every location in a file. For example, if our code is

function a(){} function b(){}

Then our function map may be

(1, 0, "a")
(1, 14, "<global>")
(1, 15, "b")

(The function name between mappings in the list is the most recent function name encountered before that location.)

We generate a function map by traversing the AST, maintaining a stack of function nodes, and recording a tuple every time we enter or exit a function. This is where we run various rules for inferring a useful name for a function given its AST node. (In Metro, we do this at the same time we transform each file with Babel, so we have easy access to the source AST.)

Step 2: Function map → encoded function map

We use a size-optimised encoding for function maps to avoid bloating the source maps unnecessarily. This is designed to be similar to how the names and mappings fields of a standard source map are encoded ( = Base64 VLQ delta coding representing offsets into a string array).

Step 3: Encoded function maps → extended source map

We collect all the encoded function maps in the x_facebook_sources field of the bundle's source map. Anywhere a source map can contain a sources field (array of file names) according to the source map spec, it can now contain an x_facebook_sources array alongside it. The fields are related by their order: The i-th source file in sources is described by the element at x_facebook_sources[i], which is itself a (one-element) array called the metadata tuple for that file.

Entry 0 in each metadata tuple is the encoded function map. No other entries are currently defined.

Step 4: Lookup in an extended source map

Starting with a location in compiled code, we find the (standard) source mapping that describes it. This gives us an offset into the sources array, as well as a line and column number. We take the same offset and look it up in the x_facebook_sources array. We decode the function map found there (caching the result for efficiency) and then do a binary search over the (line, column, name) tuples to find the nearest function name at or before the given location.

The description above is a little simplified: In Metro, we use the source-map library, which decodes mappings in a way that doesn't expose the raw offset into sources. So instead we have a helper class, SourceMetadataMapConsumer, that helps us find the right entry in x_facebook_sources for a given source file name, as well as cache, decode and query function maps.

Proposal: React Hook maps

The above solution for function maps can be adapted to implement "Hook maps", a new type of source metadata that will be understood by React DevTools.

Step 1: AST -> Hook map

  • Write a generateHookMap function that takes an AST as input and uses functions from astUtils.js to build a "hook map". Some light refactoring of astUtils.js will be needed to allow the existing logic to be reused.

If names and source locations are all we need to store, it might be possible to use the same basic data structure for the hook map as for function maps: an ordered list of (line, column, name) tuples describing where each hook starts (and ends = where the next hook starts). We can use an out-of-bounds index (e.g. -1) to represent "no hook".

The code for this should live in the React repo, probably in one of the DevTools packages.

Step 2: Hook map -> encoded Hook map

  • Either reuse the function map encoding or come up with a different encoding. The code for encoding and decoding this format should live in the React repo. (For an initial prototype, maybe we don't need any kind of clever encoding.)

Step 3: Encoded Hook maps → extended source map

  • Teach the bundler to generate and encode a Hook map for each JavaScript source file.
    In Metro, the most straightforward way to do this is to add an optional hook map to the output of every transformer, and call generateHookMap from the React Native Babel transformer to populate this data.
  • Teach the bundler to store the Hook map, if present, as entry 1 ( = the second entry) of the metadata tuple for each source file.
    In Metro, this mostly means following the existing example of how functionMap is threaded from the transformer until it's appended to x_facebook_sources.

This step will need to be done separately for each bundler we want to support, but it should reuse all the meaningful logic from steps 1 and 2. See also the "Seamless Babel integration" section below.

Step 4: Lookup in an extended source map

  • Add logic to SourceMetadataMapConsumer to decode, cache and query a Hook map. For example, we can add a reactHookNameFor({line, column, source}) method modelled on the functionNameFor() method.
  • Import the updated version of SourceMetadataMapConsumer in parseHookNames.js. Everywhere we store a SourceMapConsumer, also store a SourceMetadataMapConsumer initialised from the same map.

When implementing this, check out this comment about normalising source paths - the default logic in SourceMetadataMapConsumer matches source-map@0.5.6 and may need to be swapped out to match source-map@0.8.0-beta.0 as used in #21641.

  • Instead of always parsing the code at runtime, check if we have a Hook map first.
  • When we need to name a Hook, if we have a valid SourceMetadataMapConsumer, use its reactHookNameFor() method. Otherwise fall back to using the AST like we do now. (Maybe parse and cache the AST lazily here?)

Future: Seamless Babel integration

Writing plugins for all popular bundlers (for step 3) is doable, but getting the long tail of React developers to install these plugins - just to get a better DevTools experience for this one feature (for now) - might be difficult. Maybe doing this work for create-react-app, Next.js, React Native and a handful of other major React integrations is good enough for a start. If we want to go beyond that, it may be worth figuring out a way to build most of steps 1, 2, and 3 into @babel/preset-react.

I don't have very specific ideas here, but I think the most sustainable approach would require some modifications to Babel and to the major bundlers. Basically, we could standardise a version of x_facebook_sources, expose an API for Babel plugins to write data to it based on the source AST, and work with bundlers to propagate this information correctly when combining multiple source files into a bundle.


One possible conceptual tweak to this proposal is to store an opaque, extensible "React DevTools metadata blob" instead of specifically a "Hook map". This is kind of the same extensibility gambit I was going for with x_facebook_sources by making it an array of 1-tuples - minimising the number of times we need to re-do the same plumbing when we add a new piece of metadata. (For example, the Hermes compiler knows how to propagate x_facebook_sources opaquely, and would not need to be changed to accommodate Hook maps etc.)

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 2, 2021

Thank you for the helpful, detailed write up, @motiz88!

One thing is a little unclear to me. Are you literally suggesting that we reuse the x_facebook_sources field for this? Aside from Hermes knowing about this field, I think it would be nicer if we went with a name that is not Facebook-specific, like x_react_sources.

@motiz88
Copy link
Contributor

motiz88 commented Jul 2, 2021

x_react_sources vs x_facebook_sources is a tradeoff, but not one we have to commit to right now IMO. In the long run I totally see the value of going with x_react_sources as the thing to standardise on. I think we can still prototype this based on x_facebook_sources, which in the short term would be slightly easier to get working on FB infra (and React Native in OSS, too). To my mind it should be pretty straightforward to switch from one to the other ( / transitionally support both) as far as the DevTools codebase goes.

@mdaj06
Copy link
Contributor

mdaj06 commented Jul 3, 2021

@bvaughn @motiz88 can i have a look at this?

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 3, 2021

@mdaj06 This is a large task. If you'd be interested in picking up a piece of it, let's talk? Moti has already started some ground work (see #21790)

@mdaj06
Copy link
Contributor

mdaj06 commented Jul 3, 2021

@bvaughn Yes sure i'd be interested to take a up a small chunk of it.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 3, 2021

@mdaj06 What's your experience level with ASTs and source maps? (I ask because this issue would have to be mostly self-guided.)

Moti's comment above (#21782 (comment)) outlines a good general strategy here. Would you be interested in tackling the first item?

I believe @jstejada is also interested in working on this so we'll ideally want to coordinate any efforts between you two, if you are interested!

@mdaj06
Copy link
Contributor

mdaj06 commented Jul 3, 2021

@bvaughn i have a general understanding of ASTs and traversing them. I can research by myself for the bits where im stuck.

"Step 1: AST → function map" this would be first item, if i am not wrong? if so sure i can start looking into it.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 3, 2021

That's right, yes

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 8, 2021

@mdaj06 I posted a DevTools contributing guide this afternoon, in case it would be helpful to you.

@bvaughn bvaughn changed the title [DevTools] Improve named hooks detection [DevTools] Improve hook names performance with extended source maps Jul 8, 2021
@mdaj06
Copy link
Contributor

mdaj06 commented Jul 9, 2021

@bvaughn Sure thanks will have a look at it!

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 13, 2021

Removing myself from the Assignees list since I won't be actively working on this.

@mdaj06, if you are still working on (or interested in working on) this issue, I suggest you and Juan ( @jstejada) find some time to chat and catch up 😄 He's going pick it up this half.

@mdaj06
Copy link
Contributor

mdaj06 commented Jul 14, 2021

Yes @bvaughn im definitely interested to take it up! was held up with work in the last few days. Sure will have a chat with @jstejada over the next few days!

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 14, 2021

I'll let you two chat then.

@jstejada
Copy link
Contributor

jstejada commented Jul 16, 2021

hey @mdaj06! just wanted to check in with you on this task. I'm out next week, but I'm planning to start working on this pretty much full-time starting on July 26.

I was wondering if you already got started on this task or made any progress on any of the items above? happy to connect and sync up to figure out how best we could approach collaborating on this, if you are still interested.

thanks!

@mdaj06
Copy link
Contributor

mdaj06 commented Jul 17, 2021

Hey @jstejada i have started looking into the first item will update you on the progress soon! Let me know how we can collaborate on this task.

jstejada pushed a commit that referenced this issue Aug 11, 2021
…22010)

## Summary

Adds support for statically extracting names for hook calls from source code, and extending source maps with that information so that DevTools does not have to directly parse source code at runtime, which will speed up the Named Hooks feature and allow it to be enabled by default.

Specifically, this PR includes the following parts:

- [x] Adding logic to statically extract relevant hook names from the parsed source code (i.e. the babel ast). Note that this logic differs slightly from the existing logic in that the existing logic also uses runtime information from DevTools (such as whether given hooks are a custom hook) to extract names for hooks, whereas this code is meant to run entirely at build time, so it does not rely on that information.
- [x] Generating an encoded "hook map", which encodes the information about a hooks *original* source location, and it's corresponding name. This "hook map" will be used to generate extended source maps, included tentatively under an extra `x_react_hook_map` field. The map itself is formatted and encoded in a very similar way as how the `names` and `mappings` fields of a standard source map are encoded ( = Base64 VLQ delta coding representing offsets into a string array), and how the "function map" in Metro is encoded, as suggested in #21782. Note that this initial version uses a very basic format, and we are not implementing our own custom encoding, but reusing the `encode` function from `sourcemap-codec`.
- [x] Updating the logic in `parseHookNames` to check if the source maps have been extended with the hook map information, and if so use that information to extract the hook names without loading the original source code. In this PR we are manually generating extended source maps in our tests in order to test that this functionality works as expected, even though we are not actually generating the extended source maps in production.

The second stage of this work, which will likely need to occur outside this repo, is to update bundlers such as Metro to use these new primitives to actually generate source maps that DevTools can use.

### Follow-ups

- Enable named hooks by default when extended source maps are present
- Support looking up hook names when column numbers are not present in source map.
- Measure performance improvement of using extended source maps (manual testing suggests ~4 to 5x faster)
- Update relevant bundlers to generate extended source maps.

## Test Plan

- yarn flow
- Tests still pass
  - yarn test
  - yarn test-build-devtools
- Named hooks still work on manual test of browser extension on a few different apps (code sandbox, create-react-app, facebook).
- For new functionality:
  - New tests for statically extracting hook names.
  - New tests for using extended source maps to look up hook names at runtime.
jstejada pushed a commit that referenced this issue Aug 18, 2021
…fferent formats (#22096)

## Summary

Follow up from #22010.

The initial implementation of named hooks and for looking up hook name metadata in an extended source map both assumed that the source maps would always have a `sources` field available, and didn't account for the source maps in the [Index Map](https://sourcemaps.info/spec.html#h.535es3xeprgt) format, which contain a list of `sections` and don't have the `source` field available directly. 

In order to properly access metadata in extended source maps, this commit:

-  Adds a new `SourceMapMetadataConsumer` api, which is a fork / very similar in structure to the corresponding [consumer in Metro](https://github.com/facebook/metro/blob/2b44ec39b4bca93e3e1cf1f268b4be66f894924a/packages/metro-symbolicate/src/SourceMetadataMapConsumer.js#L56) (as specified by @motiz88 in #21782.
- Updates `parseHookNames` to use this new api

## Test Plan

- yarn flow
- yarn test
- yarn test-build-devtools
- added new regression tests covering the index map format
- named hooks still work on manual test of browser extension on a few different apps (code sandbox, create-react-app, internally).
@bvaughn bvaughn closed this as completed Sep 29, 2021
@bvaughn bvaughn changed the title [DevTools] Improve hook names performance with extended source maps Improve hook names performance with extended source maps Oct 12, 2021
zhengjitf pushed a commit to zhengjitf/react that referenced this issue Apr 15, 2022
…acebook#22010)

## Summary

Adds support for statically extracting names for hook calls from source code, and extending source maps with that information so that DevTools does not have to directly parse source code at runtime, which will speed up the Named Hooks feature and allow it to be enabled by default.

Specifically, this PR includes the following parts:

- [x] Adding logic to statically extract relevant hook names from the parsed source code (i.e. the babel ast). Note that this logic differs slightly from the existing logic in that the existing logic also uses runtime information from DevTools (such as whether given hooks are a custom hook) to extract names for hooks, whereas this code is meant to run entirely at build time, so it does not rely on that information.
- [x] Generating an encoded "hook map", which encodes the information about a hooks *original* source location, and it's corresponding name. This "hook map" will be used to generate extended source maps, included tentatively under an extra `x_react_hook_map` field. The map itself is formatted and encoded in a very similar way as how the `names` and `mappings` fields of a standard source map are encoded ( = Base64 VLQ delta coding representing offsets into a string array), and how the "function map" in Metro is encoded, as suggested in facebook#21782. Note that this initial version uses a very basic format, and we are not implementing our own custom encoding, but reusing the `encode` function from `sourcemap-codec`.
- [x] Updating the logic in `parseHookNames` to check if the source maps have been extended with the hook map information, and if so use that information to extract the hook names without loading the original source code. In this PR we are manually generating extended source maps in our tests in order to test that this functionality works as expected, even though we are not actually generating the extended source maps in production.

The second stage of this work, which will likely need to occur outside this repo, is to update bundlers such as Metro to use these new primitives to actually generate source maps that DevTools can use.

### Follow-ups

- Enable named hooks by default when extended source maps are present
- Support looking up hook names when column numbers are not present in source map.
- Measure performance improvement of using extended source maps (manual testing suggests ~4 to 5x faster)
- Update relevant bundlers to generate extended source maps.

## Test Plan

- yarn flow
- Tests still pass
  - yarn test
  - yarn test-build-devtools
- Named hooks still work on manual test of browser extension on a few different apps (code sandbox, create-react-app, facebook).
- For new functionality:
  - New tests for statically extracting hook names.
  - New tests for using extended source maps to look up hook names at runtime.
zhengjitf pushed a commit to zhengjitf/react that referenced this issue Apr 15, 2022
…fferent formats (facebook#22096)

## Summary

Follow up from facebook#22010.

The initial implementation of named hooks and for looking up hook name metadata in an extended source map both assumed that the source maps would always have a `sources` field available, and didn't account for the source maps in the [Index Map](https://sourcemaps.info/spec.html#h.535es3xeprgt) format, which contain a list of `sections` and don't have the `source` field available directly. 

In order to properly access metadata in extended source maps, this commit:

-  Adds a new `SourceMapMetadataConsumer` api, which is a fork / very similar in structure to the corresponding [consumer in Metro](https://github.com/facebook/metro/blob/2b44ec39b4bca93e3e1cf1f268b4be66f894924a/packages/metro-symbolicate/src/SourceMetadataMapConsumer.js#L56) (as specified by @motiz88 in facebook#21782.
- Updates `parseHookNames` to use this new api

## Test Plan

- yarn flow
- yarn test
- yarn test-build-devtools
- added new regression tests covering the index map format
- named hooks still work on manual test of browser extension on a few different apps (code sandbox, create-react-app, internally).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants