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

too much recursion in 2048 example code #555

Closed
wfi opened this issue Jul 16, 2015 · 10 comments
Closed

too much recursion in 2048 example code #555

wfi opened this issue Jul 16, 2015 · 10 comments

Comments

@wfi
Copy link

wfi commented Jul 16, 2015

Trying to run example code (2048) in Firefox 39.0 on Ubuntu 14.04 gives the "An unexpected error occurred: InternalError: too much recursion" message.

@blerner
Copy link
Member

blerner commented Jul 17, 2015

Hmmm, I've seen this too, on other programs; it's possible our browser detection and gas suggestion code isn't quite working any more.

@jpolitz
Copy link
Member

jpolitz commented Jul 20, 2015

I think this is just a Firefox problem. We hard-coded a stack limit of 200 in Firefox, thinking there was no way Firefox would give us fewer stack frames than that. Looks like for the shape of stuff in 2048, it does. A stack limit of 100 or less is going to run really painfully slow :-/

@schanzer
Copy link
Contributor

IIRC, some improved stack work landed a while back. Is this still considered an issue?

@blerner
Copy link
Member

blerner commented Jul 17, 2016

Well, the program no longer runs, because of syntax changes... And it prints the startup message twice, thanks to tracing... And when fixed, it runs unbelievably slowly, and there appear to be overshoot bugs in it. No idea what exactly is going on, but this bug can now be the "2048 on Firefox just doesn't work well" bug.

@jpolitz
Copy link
Member

jpolitz commented Jul 17, 2016

I fixed the syntax errors, so it loads fine.

Firefox shouldn't have an error, but I don't expect performance to be great.

I just tried it out on Chromium, and it's actually a bit slower than I remember it being, though not unusably slow. It's relatively low priority; all the BS:2 stuff runs fine (parachute, ninja-cat, etc), so I'll track down if it's performance or something with on-tick-n (I fixed a bug with that this week).

The "overshoot" actually looks quite nice when rendered quickly enough, and is intended to have the feel of a "bounce" when the tiles reach the edge.

But, this can be closed since the original source of the issue definitely isn't an issue anymore.

@jpolitz jpolitz closed this as completed Jul 17, 2016
@jpolitz
Copy link
Member

jpolitz commented Jul 17, 2016

It's slower now because there's a perf regression in TextImage, FWIW. Something about appending and removing the DOM node seems to not sit well with CPO, but is fine in WeScheme. But if you change the numbers to any other non-text shape (or omit them), it runs quite snappily no matter which browser you're in.

It's definitely creating the images (not rendering them) that takes the time, because a profile says all the time is spent in TextImage. There's a stray canvas creation in that constructor that was left in during the merge, but I removed that and it still has the issue.

I'm wondering if since CPO has more marks and DOM stuff going on in general, there's just a ton of CSS recalculation forced by the new technique, which slows things down.

@jpolitz
Copy link
Member

jpolitz commented Jul 17, 2016

Actually, this is fairly bad; it's having trouble rendering one piece of text at 28FPS. Reverting back to the old canvas strategy would work as a band-aid (though the text does render with worse baselines, overhangs, etc if we do that). Curious why this is so expensive in CPO and not WS.

Relevant code is here for reference; looks identical to WS to me (aside from the stray canvas, which I mentioned isn't the issue):

https://github.com/brownplt/code.pyret.org/blob/master/src/web/js/trove/image-lib.js#L1358

@blerner
Copy link
Member

blerner commented Jul 17, 2016

I don't understand this code at all. Why the <img> tag, why the explicit data: url and the explicit sizing, if you never reference the image again? Could decoding that data: url be the problem? Or creating the nodes and only second setting the image's display to none?

@jpolitz
Copy link
Member

jpolitz commented Jul 17, 2016

I don't fully understand it; the link to the blog is a 404, unfortunately. I assume the blog explained why the odd bits were there.

I've tried moving the display: none earlier, and changing it to position:fixed, and fixing the zIndex=-1 bit that is invalid CSS; don't see a difference.

The profile is only helpful to a point; the frame below TextImage is just anonymous function (program) which means Chrome itself. I assume that's CSS recalculation, but it could be the image decoding.

I think it's unlikely to be the image decoding because on WeScheme, this performs well.

@blerner
Copy link
Member

blerner commented Jul 17, 2016

Found it on the Internet Archive: https://web.archive.org/web/20111231045501/http://mudcu.be/journal/2011/01/html5-typographic-metrics/#baselineCanvas. Relevant bit: "By using an <img> (or any inline-block element) and the vertical-align property inside of a container element (such as a <div>), the values <canvas> provides for ctx.textBaseline can be matched with an error margin of +/- 2px—many fonts are matched exactly." And the picture they use is a 1x1.png file -- seems unlikely that the image really has anything to do with anything at all.

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

No branches or pull requests

4 participants