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

feat(v8/scripting): swap from msgpack-lite to msgpackr #3201

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thelindat
Copy link
Contributor

@thelindat thelindat commented Feb 27, 2025

Goal of this PR

Swaps out the outdated and unsupported msgpack-lite package for msgpackr, improving performance (58-95% faster depending on data and size) and adding support for more features (some are disabled for compatibility purposes).

How is this PR achieving the goal

Swaps out the custom version of msgpack-lite with msgpackr, adds bufferish to maintain backwards-compatibility for unknown types, and uses some patches to behave more like msgpack-lite.

This PR applies to the following area(s)

ScRT: JS

Successfully tested on

Game builds: FXServer 13019, FiveM 3258, RedM 1491

Platforms: Windows

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

Fixes issues

Supersedes #2931 and #3018.

@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label Feb 27, 2025
@AvarianKnight
Copy link
Contributor

Might be worth only disabling moreTypes for sessionmanager-rdr3, and adding a metadata the resource can define to turn off moreTypes and check if its set with https://docs.fivem.net/natives/?_0x964BAB1D

@thelindat
Copy link
Contributor Author

Perhaps in a separate PR which can enable moreTypes and also add some extensions that are "breaking" (e.g. vectors to array or a builtin vector class) - rather than moving the goalpost here?

@thelindat thelindat marked this pull request as ready for review February 27, 2025 16:40
@github-actions github-actions bot added ScRT: JS Issues/PRs related to the JavaScript scripting runtime invalid Requires changes before it's considered valid and can be (re)triaged and removed triage Needs a preliminary assessment to determine the urgency and required action labels Feb 27, 2025
Copy link
Contributor

@nihonium-cfx nihonium-cfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few things to improve:

  1. Add licenses, links to original and authors mentions to the files containing third-party code (as it was with msgpack-lite).
  2. Document any changes made to the third-party code so that when such code would need to get changes from the upstream - we'd know what changes to re-apply.

Nits:

  • main.js is not the most well "styled" code for sure, but it'd be great not to worsen it and keep using trailing semicolons and commas along with proper indentation for our code (third-party excluded, of course).

@nihonium-cfx nihonium-cfx added enhancement Feature or other request that adds functionality or improved usability and removed invalid Requires changes before it's considered valid and can be (re)triaged labels Feb 28, 2025
@github-actions github-actions bot added the invalid Requires changes before it's considered valid and can be (re)triaged label Feb 28, 2025
@thelindat
Copy link
Contributor Author

Licenses and links added to the built files, including a link to my fork of msgpackr so you can see the commit. I made sure to comment out the original code and include a note about msgpack-lite above the modified code; but it's fairly minimal. Some parts might not even be strictly necessary - but better safe than sorry.

I'm using the msgpackr global that's already exposed by the library, which is now removed to prevent any funny business.
msgpack_packr is removed since it doesn't serve much purpose, and I've exposed addExtension as msgpack_extend for clarity and consistency.

Copy link
Contributor

@nihonium-cfx nihonium-cfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff!

@AvarianKnight
Copy link
Contributor

AvarianKnight commented Feb 28, 2025

The original reason packr was exposed was because msgpackr supports structures, see this section, and you can add them via the packr object (iirc)

@prikolium-cfx prikolium-cfx added the ready-to-merge This PR is enqueued for merging label Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature or other request that adds functionality or improved usability invalid Requires changes before it's considered valid and can be (re)triaged ready-to-merge This PR is enqueued for merging ScRT: JS Issues/PRs related to the JavaScript scripting runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants