Skip to content

Re-use mapConsumer when resolving multiple call stacks #41

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.

Sign up for GitHub

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

Jump to bottom
Closed
mattzeunert opened this issue Oct 19, 2016 · 8 comments
Closed

Re-use mapConsumer when resolving multiple call stacks #41

mattzeunert opened this issue Oct 19, 2016 · 8 comments
Milestone
3.0

Comments

@mattzeunert
Copy link
Contributor

mattzeunert commented Oct 19, 2016 •
edited
Loading

Every time pinpoint is called a new SourceMap.SourceMapConsumer object is created. Loading the source map into the consumer object is slow, so for multiple pinpoint calls the mapConsumer should be re-used.

Possible Solution

This change could be made without any changes to the API. However, caching the mapConsumer uses a lot of memory, so maybe this behavior should be opt-in only.

Maybe a cacheMapConsumers boolean could be passed into the StacktraceGPS constructor?

I can submit a PR if we can agree on an API.
@abhinavsinghvi abhinavsinghvi mentioned this issue Oct 31, 2016
Closed
2 tasks
@abhinavsinghvi
Copy link

I was also facing similar problem, I have added #43. Let me know what do you think..

eriwen added a commit that referenced this issue Dec 11, 2016
@eriwen
Cache and reuse SourceMapConsumer for same source
e173aff 
Issue: #41
@eriwen
Copy link
Member

eriwen commented Dec 11, 2016

This is partially fixed by e173aff — we can take it a step further by splitting up _extractLocationInfoFromSourceMapSource and caching the Promise. This will prevent creating multiple SourceMapConsumers when getting mapped location from the same source file (if multiple StackFrames reference the same source file).

I'm afraid I don't have time for this refactoring tonight. Hopefully will find time soon.

@eriwen eriwen added this to the 3.0 milestone Dec 11, 2016
eriwen added a commit that referenced this issue Dec 11, 2016
@eriwen
Cache SourceMapConsumer Promise for performance
170bb4a 
This allows us to avoid multiple SourceMapConsumers for the same
source map URL.

Issue: #41
@eriwen
Copy link
Member

eriwen commented Dec 11, 2016

@mattzeunert @abhinavsinghvi I created a candidate fix that caches SourceMapConsumer Promises, but as you can see it fails in a couple edge cases on Travis CI. I'm sure I missed something silly, but I'll have to attack this another day. It would be extra cool if one of you can figure out what I missed. Just run gulp test-pr to test in real browsers locally.

@mattzeunert mattzeunert mentioned this issue Dec 14, 2016
Closed
@mattzeunert
Copy link
Contributor Author

I've reproduced and fixed the issue locally, and the PR is passing. But I think the PR tests are only a subset of the full tests from the push you referenced, so there might still be issues.

eriwen added a commit that referenced this issue Dec 17, 2016
@eriwen
Update dist/ with fixes for #41
4b035c1
@eriwen
Copy link
Member

eriwen commented Dec 17, 2016

This is fixed, as verified by tests, by merging #48. Thanks again both; this will ship in v3.0.0 soon!

@eriwen eriwen closed this as completed Dec 17, 2016
@abhinavsinghvi
Copy link

What is the plan for releasing stacktrace js with these changes? Thanks.

@eriwen
Copy link
Member

eriwen commented Jan 29, 2017 via email

@abhinavsinghvi
Copy link

abhinavsinghvi commented Mar 27, 2017 •
edited
Loading

Hi Eriwen, Please let me know if you are planning to release stacktrace js library with these changes anytime soon. Thanks,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
3.0
Development

No branches or pull requests
3 participants
@eriwen @mattzeunert @abhinavsinghvi