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

[Swift] Overall Improvements #8061

Merged

Conversation

mustiikhalil
Copy link
Collaborator

@mustiikhalil mustiikhalil commented Aug 8, 2023

The following PR adds the following:

Benchmarks are impressive compared to what we used to have:

Updated code:

running 10Strings... done! (1522.66 ms)
running 100Strings... done! (1781.41 ms)
running FlatBufferBuilder.add... done! (1542.41 ms)
running structs... done! (1478.57 ms)

Old code:

running 10Strings... done! (1422.76 ms)
running 100Strings... done! (1705.70 ms)
running FlatBufferBuilder.add... done! (2309.38 ms)
running structs... done! (2305.26 ms)
  • Upgrades minimum version of swift to 5.5
  • Allow reading unaligned buffers starting from swift 5.7, while keeping the creating aligned since it's created by the library.

Unfortunately, swift package manager doesn't allow custom compiler flags so I had to use a normal if statement to resolve this. the flag is false by default so it shouldn't be an issue most of the time, however when needed users can enable it like this:

var buffer = ByteBuffer(data: dataFromInternet)
buffer.readUnalignedBuffers = true
...

@mr-swifter @ser-0xff @hassila a quick review here would be nice!

@github-actions github-actions bot added the swift label Aug 8, 2023
@mustiikhalil mustiikhalil changed the title Allow reading unaligned buffers in swift [Swift] Overall Improvements Aug 10, 2023
@github-actions github-actions bot added the CI Continuous Integration label Aug 10, 2023
@mustiikhalil mustiikhalil force-pushed the mmk/allow-read-unaligned-buffer branch from 321ca90 to 7ce0499 Compare August 10, 2023 15:22
@github-actions github-actions bot removed the CI Continuous Integration label Aug 10, 2023
@hassila
Copy link
Contributor

hassila commented Aug 10, 2023

Out traveling - will try to have a look next week!

@hassila
Copy link
Contributor

hassila commented Aug 14, 2023

Otherwise, LGTM - on a separate note, I think it would be nice to consider moving to (https://github.com/ordo-one/package-benchmark) later on when we get 5.7 as the baseline supported and look at extending the set of benchmarks a bit, but will open a separate issue for that later when the baseline has moved...

@mustiikhalil
Copy link
Collaborator Author

@hassila correct me if I am wrong here but should it just work? Since we can configure the benchmark package to run on only 5.7?

@hassila
Copy link
Contributor

hassila commented Aug 14, 2023

Yeah, maybe you are right - but in general it's nice to have numbers that are comparable across toolchain versions too?

@mustiikhalil
Copy link
Collaborator Author

mustiikhalil commented Aug 14, 2023

Oh yeah for sure, but since benchmark package is only supporting 5.7+, we can sacrifice benchmarks on 5.5, and 5.6. So we can focus on future toolchains.

I've added it to the roadmap #6467 (comment)

@mustiikhalil mustiikhalil force-pushed the mmk/allow-read-unaligned-buffer branch from 7ce0499 to 5817560 Compare August 15, 2023 13:26
@mustiikhalil mustiikhalil force-pushed the mmk/allow-read-unaligned-buffer branch 2 times, most recently from db76c02 to c4e6b37 Compare August 18, 2023 19:10
@hassila
Copy link
Contributor

hassila commented Aug 18, 2023

Wrt package-benchmark I'd suggest adopting similar structure as is done here: apple/swift-certificates#125

Then no top level project dependency is needed and it's fine with 5.7+ now.

@mustiikhalil
Copy link
Collaborator Author

Definitely we can use that! I guess we can start working on it on a different PR

…g the creating aligned since its created by the library

Addresses a warning on xcode 15 regarding copying a pointer without safeguards

Address PR comments regarding initializing buffers with flag

Adds a test case for copying unaligned buffers

Formatting code
@mustiikhalil mustiikhalil enabled auto-merge (squash) September 27, 2023 05:48
@mustiikhalil mustiikhalil merged commit 6f71b76 into google:master Sep 27, 2023
48 checks passed
@mustiikhalil mustiikhalil deleted the mmk/allow-read-unaligned-buffer branch September 27, 2023 05:50
candysonya pushed a commit to candysonya/flatbuffers that referenced this pull request Jan 8, 2024
…g the creating aligned since its created by the library (google#8061)

Addresses a warning on xcode 15 regarding copying a pointer without safeguards

Address PR comments regarding initializing buffers with flag

Adds a test case for copying unaligned buffers

Formatting code
candysonya pushed a commit to candysonya/flatbuffers that referenced this pull request Jan 21, 2024
…g the creating aligned since its created by the library (google#8061)

Addresses a warning on xcode 15 regarding copying a pointer without safeguards

Address PR comments regarding initializing buffers with flag

Adds a test case for copying unaligned buffers

Formatting code
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.

[Swift, master] Address XC15/Swift 5.9 UnsafeRawPointer Warning
4 participants