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

Identify and fix encoding bugs #63

Merged
merged 30 commits into from
Feb 22, 2024

Conversation

mattjohnsonpint
Copy link
Collaborator

There are several cases where escape sequences and numeric exponential notation aren't being handled correctly.

Starting with a draft PR to add some tests, several of which are failing. Will follow up with some fixes to pass the tests, and then we can optimize.

@mattjohnsonpint
Copy link
Collaborator Author

Note, the failing test output is sometimes showing the wrong string. That's an as-pect bug, unrelated to this.

@mattjohnsonpint mattjohnsonpint marked this pull request as ready for review February 20, 2024 23:56
@mattjohnsonpint
Copy link
Collaborator Author

All tests are passing now. Review should be focused on perf impact, if any.

@mattjohnsonpint
Copy link
Collaborator Author

Running npm run bench:wasmtime, the main impact seems to be with Parse Object (Vec3), which lost about 75% perf. The other benchmark changes are miniscule. I'll see what I can do.

@mattjohnsonpint mattjohnsonpint marked this pull request as draft February 21, 2024 00:58
@mattjohnsonpint
Copy link
Collaborator Author

Better now. Still room for improvement though. (was -75%, now -8%)

@mattjohnsonpint mattjohnsonpint marked this pull request as ready for review February 21, 2024 03:18
@mattjohnsonpint
Copy link
Collaborator Author

Ok, I think I'm done. Here's the benchmarks:

image

Note though that I get wildly different results if I run a single benchmark vs running them all sequentially, so I'm not sure how trustworthy the benchmarks themselves are.

Nonetheless, this is about what I would expect given the changes.

@JairusSW
Copy link
Owner

Cloned and tested. Looks good and passes tests.
Let me pull and then publish as the next version

@JairusSW JairusSW merged commit 5b43b33 into JairusSW:master Feb 22, 2024
1 check passed
@mattjohnsonpint mattjohnsonpint deleted the special-encoding branch February 22, 2024 05:59
@mattjohnsonpint
Copy link
Collaborator Author

FYI, import parsing bug has been submitted separately to as-pect/visitor-as#45

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