Skip to content

Identify and fix encoding bugs#63

Merged
JairusSW merged 30 commits into
JairusSW:masterfrom
mattjohnsonpint:special-encoding
Feb 22, 2024
Merged

Identify and fix encoding bugs#63
JairusSW merged 30 commits into
JairusSW:masterfrom
mattjohnsonpint:special-encoding

Conversation

@mattjohnsonpint
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator Author

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

@mattjohnsonpint
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@mattjohnsonpint mattjohnsonpint deleted the special-encoding branch February 22, 2024 05:59
@mattjohnsonpint
Copy link
Copy Markdown
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