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

Initial gc handling implementation #520

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

KaruroChori
Copy link
Contributor

@KaruroChori KaruroChori commented May 15, 2024

Initial implementation. Not to be merged!
It required me patching quickjs to add a getter for the threshold, as that is currently only exposed via a setter.

There is something I don't particularly like: this is the formula used to update it:

        rt->malloc_gc_threshold = rt->malloc_state.malloc_size +
            (rt->malloc_state.malloc_size >> 1);

Basically we have no control over its rescaling after a GC event is run. I think there should be a configurable flag to determine if such threshold can be rescaled. This would allow for a wider range of control strategies to be implemented.

From a high level perspective these would be set with tjs.gc.strategy. Proposed scaling heuristics for now:

  • default the self scaling mechanism used above
  • always forcing GC events at each allocation
  • fixed the threshold will not be rescaled after the event

I am going to open the relevant issues on the quickjs repo for this as well as the callbacks.

@KaruroChori
Copy link
Contributor Author

By the way, how is linting handled in this project? It seems it does not automatically integrate with my current setup.

@saghul
Copy link
Owner

saghul commented May 15, 2024

I think it's better to just set the threshold at the beginning ourselves, so we don't need a getter.

If / when there is a getter we can change that.

Also, please drop the not yet implemented handlers, we'll add them once QuickJS has them.

Regarding strategy, right now there is not much control from outside QuickJS, so I wouldn't make promises on our end just yet!

@saghul
Copy link
Owner

saghul commented May 15, 2024

By the way, how is linting handled in this project? It seems it does not automatically integrate with my current setup.

It's a manual step, npm run lint

@KaruroChori
Copy link
Contributor Author

KaruroChori commented May 16, 2024

Sorry for the misunderstanding, this was not meant as code to be merged at this stage.
I just wanted to identify what was needed in terms of features and reach consensus upon an interface, so that we don't have to go back and forth changing it too much later :D.

Since I was in the mood I finished implementing everything on both txiki and quickjs. So there are now proper callbacks, fixable thresholds, getters and from the brief tests I made it seems to be working fine.
At least this is running as expected:

const v = []
let i = 0;
tjs.gc.onBefore = ()=>{console.log("BEFORE!", i);return true;}
//tjs.gc.onAfter = ()=>{console.log("AFTER!", i);}
tjs.gc.threshold = 0;
for(; i < 50000000; i++){
    if(i% 1000000 === 0)console.log(`[${tjs.gc.threshold}]`)
    v.push(i);
}
let p = 0;
for(; i < 50000000; i++){
    p+=v[i]
}

Tests are missing, but I will only write the once these or similar features are agreed upon in both repositories.

Clearly a separate branch for quickjs was made for that here.

Now that the code is there I will create a separate branch and pr specific for the kind of integrations which can be made using mainline quickjs. Ideally as things progresses on their repo I will be moving over more and more features from the "complete" branch based on your reviews and with the additional tests and documentation needed.

@KaruroChori
Copy link
Contributor Author

By the way, how is linting handled in this project? It seems it does not automatically integrate with my current setup.

It's a manual step, npm run lint

For future reference, npm run lint -- --fix to also apply them. I never used lint this way, so there was a bit of confusion :D.

@KaruroChori
Copy link
Contributor Author

As for what I can bring to txiki right now based on the current quickjs master branch:

  • The enable flag
  • The threshold variable, knowning that when it will be changed by the dynamic scaling formula after GC is called, it will end up out of sync. The getter was needed to ensure consistency.

If you agree, I will open a new (not draft) PR for with these 1.5 features.

@saghul
Copy link
Owner

saghul commented May 16, 2024

As for what I can bring to txiki right now based on the current quickjs master branch:

  • The enable flag

  • The threshold variable, knowning that when it will be changed by the dynamic scaling formula after GC is called, it will end up out of sync. The getter was needed to ensure consistency.

If you agree, I will open a new (not draft) PR for with these 1.5 features.

Yeah let's do the 1.5 :-)

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.

None yet

3 participants