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

Simplify/update fxparam deserializers & boilerplate #26

Closed

Conversation

postspectacular
Copy link

@postspectacular postspectacular commented Apr 14, 2023

This is a companion PR for fxhash/fxlens#18 reflecting the same updates as done in there (plus some additional minor refactoring)...

The original incentive here was to reduce file size (currently 20% smaller), but not doing so for "size coding" reasons, but to simplify and improve internal re-use (incl. the fixes mentioned in the other PR).

There's one thing I'm unsure about, both here and in the other PR: I don't understand why your byteLength() method for strings always returns the value-times-two. My hunch is that this was your attempt to address the fact that some unicode chars require multiple bytes and so that factor 2 acts as a reserve. If that's the case, then I think that new route using TextEncoder doesn't require it that double size anymore. Also in my extensive testing with the updated fxlens I couldn't find any issues (yet)... But maybe I'm misunderstanding...

Anyhow, hope these are useful & acceptable updates! Happy to make further changes if needed...

- replace remaining single quote occurrences w/ double quotes
- remove remaining semicolons
* upstream/master:
  removed iteration number injection
  add minter & iteration number
  add version; deprecate old events for single getInfo event
  add 2nd long string
  comment fx preview event dispatch
  remove dislcaimer
  allow user defined max length
- add rndHash() & matcher() helpers
- improve internal re-use for fxhash & fxminter init
- undoing previous megabrain blooper
@maerzhase maerzhase self-requested a review April 16, 2023 09:46
@maerzhase maerzhase self-assigned this Apr 16, 2023
@maerzhase
Copy link
Contributor

@postspectacular man, thank you so much for your input and your thorough improvements, we are really grateful for the time you spend with our implementation 💘

There's one thing I'm unsure about, both here and in the other PR: I don't understand why your byteLength() method for strings always returns the value-times-two. My hunch is that this was your attempt to address the fact that some unicode chars require multiple bytes and so that factor 2 acts as a reserve. If that's the case, then I think that new route using TextEncoder doesn't require it that double size anymore. Also in my extensive testing with the updated fxlens I couldn't find any issues (yet)... But maybe I'm misunderstanding...

you are exactly right. all chars are encoded into 4 bytes ultimately resulting in bytelength times two. wasn't a masterpiece xD
your approach of using TextEncoder seems just perfect, I must admit that I wasn't aware of it 👍

we are going to checkout your improvements in the upcoming days and do some test with existing tokens etc on our end, so we can integrate these improvements soon!

much love and appreciation from the whole fx(hash) team!!!

@maerzhase maerzhase changed the base branch from master to feature/data-emission June 16, 2023 11:24
maerzhase added a commit that referenced this pull request Jun 16, 2023
@maerzhase maerzhase changed the base branch from feature/data-emission to feature/serialisation-improvements June 16, 2023 12:48
@maerzhase maerzhase changed the base branch from feature/serialisation-improvements to feature/data-emission June 16, 2023 12:50
@maerzhase
Copy link
Contributor

couldn't push on your branch. changes are incorporated in fxhash/fxlens#22

@maerzhase maerzhase closed this Jun 16, 2023
maerzhase pushed a commit that referenced this pull request Oct 1, 2023
Fix local builds aka update dependencies
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