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.

Sign up for GitHub

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

Jump to bottom

Make readInt() 3-4 times faster #284

Merged
merged 2 commits into from
Dec 6, 2021
Merged

Make readInt() 3-4 times faster #284

merged 2 commits into from
Dec 6, 2021

Conversation

juselius
Copy link
Contributor

@juselius juselius commented Dec 2, 2021

readint () uses a local temporary array for endian conversion. This array gets created and garbage collected for every value, putting a lot of strain on the garbage collector. By making the array a reusable class variable, we achieve 3-4 times faster execution. The class variable is only used by Fable, and since JavaScript is single-threaded there should not be any issues related to race conditions etc.

@juselius juselius mentioned this pull request Dec 2, 2021
Closed
@kerams
Copy link
Collaborator

kerams commented Dec 2, 2021

Ah, you found perhaps the lowest hanging fruit it would seem :). Very nice.

@kerams kerams self-requested a review December 2, 2021 10:49
@kerams
Copy link
Collaborator

kerams commented Dec 2, 2021 •
edited
Loading

You know, on second thought it might good to do the same thing on .NET too. The in-place reverse with Array.Reverse is dirty because it mutates the input array if the system is little endian. I would not worry about threading issues on .NET because the Reader type is not designed to be thread-safe (pos is already a mutable instance property), though it would be problematic should we ever use parallelization during a single deserialization (in your original data model, both the integer and float arrays in a tuple could be processed simultaneously).

What do you think?

@juselius
Copy link
Contributor Author

juselius commented Dec 2, 2021

@kerams since multi-threading and parallelization isn't a concern, it's a trivial fix. Just remove the special case for !FABLE_COMPILER, and it's done :) I'll make a new PR!

@juselius
Copy link
Contributor Author

juselius commented Dec 2, 2021

Shall I bump the version to 1.13.1 at the same time?

@kerams
Copy link
Collaborator

kerams commented Dec 2, 2021 •
edited
Loading

Shall I bump the version to 1.13.1 at the same time?

There's no need.

@Zaid-Ajaj Zaid-Ajaj merged commit 5d0d172 into Zaid-Ajaj:master Dec 6, 2021
@Zaid-Ajaj
Copy link
Owner

Thanks a lot @juselius and @kerams I will publish a new package soon!

Zaid-Ajaj added a commit that referenced this pull request Dec 7, 2021
@Zaid-Ajaj
Fable.Remoting batch with improved readInt() perf (#284) and Recyclab…
a1ed230 
…leMemoryStream option (#280)
@Zaid-Ajaj
Copy link
Owner

@juselius A new batch of packages has been publishes that includes the improvement in MsgPack 🚀 updating Fable.Remoting.Client should give you latest bits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kerams kerams kerams approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@juselius @kerams @Zaid-Ajaj