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

POC - Deduplicating shared constants #138

Closed

Conversation

dawsbot
Copy link

@dawsbot dawsbot commented Jun 5, 2022

This is a POC that takes idna-map.min.js from 166,718 bytes down to under 100,000 bytes

This first change brought it down to 144296

In a critical effort to reduce bundle sizes, I thought it might be good to show an example of an easy way to do this. I see discussions and issues are both disabled for this repo, so there is not a better place to propose changes besides a PR.

Here was the simple change I made, then did a find-and-replace:

image


EDIT 1

I moved a few more of the most-commonly used constants up and out. Now idna-map.min.js is 105980 bytes


EDIT 2

I tested an array map function for filling large duplicated subsections with good success:

// fills var "b" 1785 times
.concat(Array.apply(null, Array(1785)).map(function(){return b})).concat([

@dawsbot dawsbot changed the title POC - Deduplicating shared numbers POC - Deduplicating shared constants Jun 5, 2022
@KaiSchwarz-cnic
Copy link
Contributor

@dawsbot I enabled issues - missed this in the past
I'll have an eye on your findings soon, actually highly busy.

@KaiSchwarz-cnic
Copy link
Contributor

KaiSchwarz-cnic commented Jul 4, 2022

Hey @dawsbot,

I am enjoying your valuable ideas. In lack of available work time, I've had an eye on what I can provide in short term.
#142 is introducing the const var for Uint32Array.

idna-map.js is getting build by build-unicode-tables.py. That means the js code as string is getting build there and finally saved as file. It has to come with a generic solution for getting the values replaced with single const vars (or an array of values). Actually, this is a bit more than I can provide as it needs some deep rethinking and I have to argue why I am working on this project as it actually has lower priority than others assigned. ;-)
So, for now please enjoy the merge of #142. You'll hear back from me regarding the rest.

That said, 4.0.3 has been rolled out.

HTH

@dawsbot
Copy link
Author

dawsbot commented Jul 5, 2022

Hey @KaiSchwarz-cnic I made a fork where I've done more than one week full-time to recreate this python script with JS. You can see the script here and the rest of the repo at https://github.com/dawsbot/idna-uts46

I'm still fiddling with the build optimizations more before pushing this out to a few important packages in web3 which rely on this package.

The world of Ethereum desperately deserves smaller bundle sizes so I figured it's best if a more active developer upkeeps the standard library we share. I'm happy to join forces if you're able to respond and check on this package more often.

@dawsbot dawsbot closed this Jul 5, 2022
@KaiSchwarz-cnic
Copy link
Contributor

@dawsbot no problem at all, that's what I did in the past as well as the original project wasn't maintained any longer. I am an highly active developer, just cannot spent huge time actually on this repository. Still, what you covered had been in our backlog - rewriting in direction of JS. Impressive that you came there. Still, this library is also important for us for doing idn to punycode conversions as fast as possible. Maybe we should discuss how to work closely together? I could make you to a contributor, if that's fine for you? Or vice-versa? I am fine with handing the project over to you - but basically you published your fork already at npmjs. It just doesn't make sense to continue with both packages in parallel.

@dawsbot
Copy link
Author

dawsbot commented Jul 5, 2022

@KaiSchwarz-cnic This collaboration sounds good. I have not yet published to npm, I just made the documentation as if that was done already.

Let's find a more effective way to DM on this. Twitter or email (you already have my email from a few months ago 🙏 )

@KaiSchwarz-cnic
Copy link
Contributor

KaiSchwarz-cnic commented Jul 6, 2022

@dawsbot yes, I also highly enjoy the idea of a working-together. Let me invite you as contributor =)

Just one note: We use commitizen to keep our commit messages standardized following the angular style preset.
Our release process auto-runs on master when a merge of commits happens with a specific types mentioned in the commit message.

First at all, using the latest git version is a must for us, otherwise the below git workflow won't be working. So, preparatory work would be:

sudo add-apt-repository ppa:git-core/ppa
sudo apt update
sudo apt install git
npm i --location=global commitizen cz-conventional-changelog
touch ~/.czrc

then put the below content into ~/.czrc:

{
  "path": "cz-conventional-changelog"
}

If you want to commit to your custom branch just do:

git add .
cz
git push

This shows up a wizard helping you with the angular preset format.

Git Development Process:

// update your local branch
git fetch origin -p --tags && git pull


// rebasing your custom branch to master (ensure to have updated your master with prev. line)
git rebase master
git push -f


// interactive rebase for cleaning up your commit history in your local branch
git rebase -i HEAD~<number of commits in your branch>
// ... then replace 'pick' with the actions you want to cover (reword, fixup are the common ones we use) for the commits of interest and save
// git will lead you then to further steps if necessary (allowing for rewording commits you specified for example, or reviewing the commit message in case of squash)
// squash / fixup happens from bottom to top
git push -f

We are also using zsh and oh-my-zsh with a custom setup to make the git status being reflected in shell as well.
Let me know if we shall share this as well with you.

HTH 🙋

@dawsbot
Copy link
Author

dawsbot commented Jul 10, 2022

@KaiSchwarz-cnic I realize you've made these steps in-order to maintain a specific pattern you like in your codebase, but those steps are extremely daunting to contribute.

I've already made a lot of progress over on my branch, would you want me to contribute those back here?

The last two PR's I submitted you fully reworked on your own and I ended up with zero commit history in master anyways.

@KaiSchwarz-cnic
Copy link
Contributor

@dawsbot no worries. I am interested in coming to your solution. I am aware of that this also comes with rewriting our current workflows. I suggest to open a PR with your changes and that you describe the build steps for the idna map file etc. I'll then be rewriting gh action workflows.

Hope this sounds good?

Kai

@KaiSchwarz-cnic
Copy link
Contributor

@dawsbot if you prefer it, not sure about your thoughts - I can of course be working on the PR as well. I am a friend of "honor to whom honor is due" and to stick to it in this working-together. Still, I am not yet aware of your thoughts how you imagine us to work side-by-side.

@KaiSchwarz-cnic
Copy link
Contributor

As we did not receive feedback from you @dawsbot, aren't you any longer interested in joining us as contributor?

@dawsbot
Copy link
Author

dawsbot commented Aug 6, 2022

@KaiSchwarz-cnic I don't see myself having time for this.

I've done a lot of the hard-work to rewrite this in JS but with the complexity of your contribution guidelines I've lost all interest. Consider pulling in the changes you like: https://github.com/dawsbot/idna-uts46

@KaiSchwarz-cnic
Copy link
Contributor

KaiSchwarz-cnic commented Mar 9, 2023

@dawsbot covered. Thanks for the hard work, very much appreciated!
Published to npmjs.org as version 5.0.0.

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

Successfully merging this pull request may close these issues.

2 participants