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

[Swift] Push contiguous bytes #8157

Merged
merged 20 commits into from
Nov 20, 2023
Merged

[Swift] Push contiguous bytes #8157

merged 20 commits into from
Nov 20, 2023

Conversation

wtholliday
Copy link
Contributor

I noticed that saving in my app was quite slow as each individual byte of a byte buffer was treated generically as a Scalar:

image

This change uses ContiguousArray to optimize that. Runtime went from 1s to 2ms. I'm not sure what the exact API should be.

@google-cla Google CLA
Copy link

google-cla bot commented Nov 12, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added the swift label Nov 12, 2023
@wtholliday wtholliday changed the title Push contiguous bytes [Swift] Push contiguous bytes Nov 12, 2023
@mustiikhalil
Copy link
Collaborator

mustiikhalil commented Nov 12, 2023 •
edited
Loading

@wtholliday Hey, thanks for your contribution.

Would love to point out it would be nice to add

1- A benchmark test to show that the code that's written has a better performance than master and let's keep the benchmark committed for future use.

It would be a simple test, where we take in an array of ContiguousBytes with size 1000 and run a benchmark before the changes that's similar to the string with a 100 size benchmark. Then we run the same benchmark after the changes and post the findings in the PR.

2- Tests to verify that you can read the code back. In FlatBuffersMonsterWriterTests.swift create a new function that writes a new monster but instead of using the normal create vector use the new one that you created and then try reading it.

@mustiikhalil
Copy link
Collaborator

@wtholliday continuation of my thought process, sorry for not including it earlier

3- It would be nice to investigate if it's possible to actually completely migrate the createVector function into similar one to the one you wrote.

4- Instead of using

_storage.memory
         .advanced(by: writerIndex &- ptr.count)
         .copyMemory(from: ptr.baseAddress!, byteCount: ptr.count)

can you please use memcpy it would be similar to this:

    memcpy(
      _storage.memory.advanced(by: writerIndex &- len),
      UnsafeRawPointer(bytes.baseAddress!),
      len)
    _writerSize = _writerSize &+ len

@wtholliday
Copy link
Contributor Author

Going to wait for a few other fixes to land, then I'll get back to this :)

@mustiikhalil
Copy link
Collaborator

Looks great!

What is missing is the tests to verify we can write and read properly.

Furthermore, optionally, do you still want to add the changes to the other vector function?

@wtholliday
Copy link
Contributor Author

Yep still planning to add tests. I will also see what I can do about converting the existing API

@mustiikhalil
Copy link
Collaborator

@wtholliday Perfect! cause we are planning on having a release soon so it would be amazing to have the changes for everyone 😄

@wtholliday
Merge branch 'google:master' into push-ContiguousBytes
7731296
@wtholliday
Update push to copy memory
9504afc 
Since we don't care about endianness, we can simply memcpy the array of scalars
@wtholliday
Remove function and benchmarks
a78dd2e 
Since we don't care about endianness, a FixedWidthInteger version of createVector isn't needed
@wtholliday
Improve naming
bc2ddb9
mustiikhalil
mustiikhalil approved these changes Nov 20, 2023
View reviewed changes
Copy link
Collaborator

@mustiikhalil mustiikhalil left a comment

Choose a reason for hiding this comment

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

LGTM

@mustiikhalil mustiikhalil merged commit b08abbb into google:master Nov 20, 2023
48 checks passed
@mustiikhalil mustiikhalil mentioned this pull request Nov 20, 2023
Merged
@hassila hassila mentioned this pull request Nov 21, 2023
Closed
candysonya pushed a commit to candysonya/flatbuffers that referenced this pull request Jan 8, 2024
@wtholliday @candysonya
[Swift] Push contiguous bytes (google#8157)
6450bb8 
* Add version of push which takes ContiguousBytes

* Ensure overloads aren't ambiguous

* Add version of createVector

* Add version of push which takes ContiguousBytes

* Ensure overloads aren't ambiguous

* Add version of createVector

* Add similar conditional to other use of ContiguousBytes

* Attempt CI fix

* Use memcpy instead of copyMemory

memcpy is faster in tests

* Add testContiguousBytes

* Add benchmarks

* Add version of createVector

* Add benchmarks

* Update push to copy memory

Since we don't care about endianness, we can simply memcpy the array of scalars

* Remove function and benchmarks

Since we don't care about endianness, a FixedWidthInteger version of createVector isn't needed

* Improve naming

* Add doc comment
@BrewTestBot BrewTestBot mentioned this pull request Mar 8, 2024
Merged
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
@wtholliday
[Swift] Push contiguous bytes (google#8157)
5c8c610 
* Add version of push which takes ContiguousBytes

* Ensure overloads aren't ambiguous

* Add version of createVector

* Add version of push which takes ContiguousBytes

* Ensure overloads aren't ambiguous

* Add version of createVector

* Add similar conditional to other use of ContiguousBytes

* Attempt CI fix

* Use memcpy instead of copyMemory

memcpy is faster in tests

* Add testContiguousBytes

* Add benchmarks

* Add version of createVector

* Add benchmarks

* Update push to copy memory

Since we don't care about endianness, we can simply memcpy the array of scalars

* Remove function and benchmarks

Since we don't care about endianness, a FixedWidthInteger version of createVector isn't needed

* Improve naming

* Add doc comment
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
@wtholliday
[Swift] Push contiguous bytes (google#8157)
ca120fd 
* Add version of push which takes ContiguousBytes

* Ensure overloads aren't ambiguous

* Add version of createVector

* Add version of push which takes ContiguousBytes

* Ensure overloads aren't ambiguous

* Add version of createVector

* Add similar conditional to other use of ContiguousBytes

* Attempt CI fix

* Use memcpy instead of copyMemory

memcpy is faster in tests

* Add testContiguousBytes

* Add benchmarks

* Add version of createVector

* Add benchmarks

* Update push to copy memory

Since we don't care about endianness, we can simply memcpy the array of scalars

* Remove function and benchmarks

Since we don't care about endianness, a FixedWidthInteger version of createVector isn't needed

* Improve naming

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

@mustiikhalil mustiikhalil mustiikhalil approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@wtholliday @mustiikhalil