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

Share BytesStarts across frames in flamegraph #41

Closed
jonhoo opened this issue Feb 8, 2019 · 10 comments
Closed

Share BytesStarts across frames in flamegraph #41

jonhoo opened this issue Feb 8, 2019 · 10 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@jonhoo
Copy link
Owner

jonhoo commented Feb 8, 2019

While #37 got us pretty far, there is still one source of allocation in the main flamegraph loop: the attributes on BytesStart. Specifically, when you add attributes to a BytesStart, it allocates a Vec to hold the underlying bytes if it doesn't already have a Vec. That's unfortunate since we create a new BytesStart for every g. If we instead created a single BytesStart that we re-used somehow (by resetting all the attributes), that'd be way more efficient. Sadly, that's not yet supported, but I've filed tafia/quick-xml#148 which would enable us to do that!

@jonhoo jonhoo added enhancement New feature or request help wanted Extra attention is needed labels Feb 8, 2019
@meven
Copy link
Contributor

meven commented Feb 15, 2019

FYI, I have opened a PR in quick-xml repo tafia/quick-xml#149 to fix tafia/quick-xml#148

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 19, 2019

Now that tafia/quick-xml#149 has landed, this should be easy-peasy! :D

@meven
Copy link
Contributor

meven commented Feb 19, 2019

Now that tafia/quick-xml#149 has landed, this should be easy-peasy! :D

Well we just need to wait for a quick-xml release now.

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 19, 2019

@meven I would start working with quick-xml as a git dependency, and then we can just hold off on releasing until a new release of quick-xml is made :)

@meven
Copy link
Contributor

meven commented Feb 19, 2019

Having a look unfortunately Event::Start consumes the BytesStart.
We will need a clone() unfortunately.

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 19, 2019

Ah, that's unfortunate. The clone will at least mean that we're unlikely to do multiple memory allocations though. @tafia any idea how we may be able to do this without allocation, given that Event;;Start consumes BytesStart (as opposed to taking a &BytesStart)?

@tafia
Copy link

tafia commented Feb 20, 2019

Event is the primary enum returned in quick_xml. I think it wouldn't make sense for it not to own its content (in particular when reading the xml).

On the other hand, Writer::write_event takes a AsRef<Event> but you're using write_event(event).

So if instead you decide to use write_event(&event), you could then get back the event (if let Event(start) = event) and do whatever you need.

I haven't tried it on your code but it should work.

jonhoo pushed a commit that referenced this issue Feb 20, 2019
This saves us some allocations on every frame and fixes #41.
@jonhoo
Copy link
Owner Author

jonhoo commented Feb 20, 2019

We should also do this for the rect and a elements, since they're also in the main loop. /cc @meven

@jonhoo jonhoo reopened this Feb 20, 2019
@jonhoo jonhoo changed the title Share BytesStart across gs in flamegraph Share BytesStarts across frames in flamegraph Feb 20, 2019
@jonhoo
Copy link
Owner Author

jonhoo commented Feb 20, 2019

I would probably work on this on top of #32 rather than current master since it's about to be merged, and moves a lot of code around.

@meven
Copy link
Contributor

meven commented Feb 20, 2019

I would probably work on this on top of #32 rather than current master since it's about to be merged, and moves a lot of code around.

Good suggestion, I can do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants