Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

ZoneAwareError is 15-20x slower than native, should lazy-evaluate "stack" #975

Closed
mleibman opened this issue Dec 5, 2017 · 8 comments · Fixed by #1045
Closed

ZoneAwareError is 15-20x slower than native, should lazy-evaluate "stack" #975

mleibman opened this issue Dec 5, 2017 · 8 comments · Fixed by #1045

Comments

@mleibman
Copy link
Contributor

mleibman commented Dec 5, 2017

Getting a stack trace can be an expensive operation, and both Firefox and Chrome/V8 (haven't checked IE/Edge) optimize it by formatting the unstructured stack trace of an error on demand, the first time the stack property is accessed (https://github.com/v8/v8/wiki/Stack-Trace-API#customizing-stack-traces).

When building instrumentation tooling, I use this to minimize the overhead of capturing the stack trace using new Error() and then evaluate its stack property if and when needed. Unfortunately, ZoneAwareError does the trace evaluation and formatting in its constructor, so it ends up a lot slower, both from invoking the native error's formatting, and Zone.js's own overhead. I have also noticed it resulting in increased memory pressure, leading to more frequent GC pauses:

zoneawareerror

(The first half is running new NativeError(), the second is running new Error().)

Benchmark demonstrating the issue - https://jsperf.com/zoneawareerror.
Note that this runs the benchmark with an unrealistically shallow call stack. The real life impact in a complex application is likely to be more pronounced.

Is it possible to make it evaluate and rewrite the stack on demand?

@JiaLiPassion
Copy link
Collaborator

@mleibman , so your requirement is

  1. use native Error instead of zone patched one?
  2. use zone patched one, but format stack on demand?

@mleibman
Copy link
Contributor Author

mleibman commented Dec 5, 2017

I can use the native error as a workaround, though there doesn't seem to be a formal way of getting it (api.symbol('Error') is private, and window['__zone_symbol__Error'] is hacky and not guaranteed to work).

Ideally, the zone patched one should not incur any additional overhead in the constructor, and do all its work on demand in the stack accessor. Using the zone patched error is preferable since it filters out the blacklisted zone frames and adds zone information for each one.

@JiaLiPassion
Copy link
Collaborator

@mleibman ,

  1. if you want to use native error, you can update to latest version of zone.js, and by default,
    node_modules\dist\zone-error.js is not loaded, so you will get native Error automatically.
  2. if you use some old version of zone.js, you can set __Zone_disable_Error = true; in index.html before loading zone.js.
  3. if you want to use zone patched Error, the blacklisted zone frame filter can be attached on demand, but zone information on stack frame can't be attached on demand, because the zone also keep a stack frame which only hold correct information when Error is constructed.

@mleibman
Copy link
Contributor Author

mleibman commented Dec 5, 2017

@JiaLiPassion,

1 & 2 aren't really an option for me.

But it seems that we can still have our cake and eat it too. I may be reading the code wrong, but there doesn't seem to be anything in the ZoneAwareError constructor that needs to actually access the stack property right away. The zone information for the current stack frame (as read in let zoneFrame = api.currentZoneFrame()) can be captured in the constructor and used on-demand later. What do you think?

Also, is there a better way to access the native Error other than using window['__zone_symbol__Error']?

@JiaLiPassion
Copy link
Collaborator

yes, we can keep the zoneFrame in Error constructor and build the stack on demand, but not only keep the zoneFrame itself, but also all it's ancestors. I will try to create a patch.

to access native Error, you need to access window['__zone_symbol__Error'] or window[Zone.__symbol__('Error')], what is your suggestion?

@mleibman
Copy link
Contributor Author

mleibman commented Dec 5, 2017

@JiaLiPassion,

If you capture the currentZoneFrame, does that not give you access to all of its ancestors too via the currentZoneFrame.parent? It doesn't seem like you have to do anything special there, but again, I'm not familiar with the code, so this is all based on just reading through it.

The reason I'm asking about the proper way to access the native Error is that I'm concerned that it may stop working at some point. The api.symbol() (also referenced as Zone.__symbol__) looks like a shim for ES6 Symbol functionality that just prepends it with '__zone_symbol__' until ES6 symbols are universally supported. Right now, the shim is used regardless of whether the environment supports ES6 symbols, but if Zone switches to actually using native ES6 symbols at some point, window['__zone_symbol__Error'] will stop working. The other problem is that the '__zone_symbol__' prefix is an internal implementation detail, and Zone.js makes no guarantees that it won't change.

UPDATE: Sorry, I missed the window[Zone.__symbol__('Error')] option. That addressed the "internal implementation detail" part, and should be fine as long as Zone.__symbol__('Error') remains available.

@JiaLiPassion
Copy link
Collaborator

@mleibman , yes, the currentZoneFrame will include all it's ancestors information in Error constructor, but if we do so, it will keep all zone information in Error and may cause memory leak. I will think about it again.

And about the native Error access, I am not sure it will change or not. Maybe we should provide a getNative method.

@mleibman
Copy link
Contributor Author

mleibman commented Dec 5, 2017

Thanks!

JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 20, 2018
JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 20, 2018
JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 20, 2018
JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 20, 2018
JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 21, 2018
JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 22, 2018
JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 22, 2018
JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 22, 2018
JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 22, 2018
JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants