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

Reduce garbage generation in InputHandler.print #1817

Closed
wants to merge 1 commit into from

Conversation

juancampa
Copy link
Contributor

Problem

Profiler was showing Minor GC triggering because of getCharAt. Which means these 1-char are add GC pressure.

Solution

Reuse 1-char strings by keeping them in a cache.

Tests

I ran a few sanity checks and everything seems to be working fine. The profiler also doesn't show the GC firing to collect these anymore.

Gonna post profiler screenshots to illustrate the problem after the break :)

By keeping a cache of characters instead of using getCharAt
@juancampa
Copy link
Contributor Author

Hmm 🤔 I can't seem to reproduce what I was seeing earlier so I'm gonna go ahead and close this until I can repro it

@juancampa juancampa closed this Dec 8, 2018
@jerch
Copy link
Member

jerch commented Dec 8, 2018

@juancampa This highly depends if you are using the JSArray or TypedArray buffer:

  • the JSArray holds the char in the buffer - long living object (expensive to collect for GC)
  • the TypedArray has it only as stack variable, it does not outlive the function body (short living and easier to cleanup)

For both it adds GC pressure and for the TypedArray it is even a nonsense conversion and only there for API conformance with the JSArray buffer. I plan to address this once we got rid of the JSArray buffer version (see #1796 and #1811).

@juancampa
Copy link
Contributor Author

You're right, this will go away when your TypedArray PR goes in.

Additionally I'm seeing that at least V8 has an internal cache for one-byte strings: https://github.com/v8/v8/blob/master/src/code-stub-assembler.cc#L6661-L6664, though it's not clear if it's being used (e.g. I can't find the definition of kSingleCharacterStringCache in the V8 repo, so maybe it's dead code)

Okay then, I'm not bothering anymore with this PR. Thanks for the clarification @jerch

@jerch
Copy link
Member

jerch commented Dec 10, 2018

Yeah v8 has the ability to "internalize" often used short strings, this turns a JS string basically into a char pointer under the hood (and makes things much more performant). I think they use some heuristics to determine whether to do the internalization, and I think we typically dont hit it during input flow (as the single char string change lot).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants