[Swift] Add allowReadingUnalignedBuffers to most ByteBuffer init methods #8134

abandy · 2023-10-26T14:04:17Z

This change is for issue #8133. It adds allowReadingUnalignedBuffers to the ByteBuffer init methods that accept the buffers data as an input.

google-cla · 2023-10-26T14:04:21Z

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.

mustiikhalil · 2023-10-26T14:48:15Z

@abandy was there a case where you needed this specific implementation? If so can you provide an example?

abandy · 2023-10-26T16:11:17Z

@abandy was there a case where you needed this specific implementation? If so can you provide an example?

Hi @mustiikhalil, I hope all is well. In swift arrow we have a method that creates a ByteBuffer from data which can be unaligned. It would be less overhead if I can just create the ByteBuffer from the unaligned Data struct.

mustiikhalil · 2023-10-26T19:11:46Z

Hey @abandy

In swift arrow we have a method that creates a ByteBuffer from data which can be unaligned. It would be less overhead if I can just create the ByteBuffer from the unaligned Data struct.

Makes sense. We can definitely add this! I guess it would be benefitial for alot of people. However to be completely transparent with you, there is also an overhead reading an unaligned buffer which you can also revisit and see if that's something type of overhead you would be willing to take. Which would lead to either of these three options:

Realign the buffer which will be an overhead from your end. so a buffer that's contains one object can be copied and read directly.
Use the newer methods of reading instead of let message = org_apache_arrow_flatbuf_Message.getRootAsMessage(bb: mbb) which would look like the following & I also linked a reference to the tests:

do {
  let message: org_apache_arrow_flatbuf_Message.Message = try getCheckedRoot(from: &buffer) // dont remember the correct syntax on the top of my head but it would look like something like this
} catch {
 print("unaligned buffer")
}

ref:

flatbuffers/tests/swift/tests/Tests/FlatBuffers.Test.SwiftTests/FlatbuffersVerifierTests.swift

Line 59 in 205285c

func testVerifierCheckAlignment() {

Use the unaligned copy which will copy on every field that it would try to read in the case of the switch statement it would be around three times and the more you read the move the overhead will be.

As i mentioned above we can merge this. However, since performance is what you need, it would make more sense if we can verify the three options with benchmarks. Reading an unaligned buffer, realigning a buffer and a verified buffer (Personally, i havent used the verifier before so it would be interesting to see its performance in general)

abandy · 2023-10-26T22:42:55Z

Hi @mustiikhalil, the unaligned buffer error is fatal so catching it is not an option (AFAIK). I am planning to have the user elect to use the unaligned buffer and will not set it by default, that way the user can decide to take the performance hit or align the buffer if they chose.

If we could merge this change that would be great!

I have tried the verifier but it doesn't fail for me with the overall buffer but there is a header in the message buffer that fails afterward. When I have sometime I will investigate, as it will allow for a non fatal approach.

Also, is it possible to get a new version released? It seems that the last version for the swift flatbuffers was prior to your last changes.

mustiikhalil · 2023-10-26T23:35:19Z

@abandy

the unaligned buffer error is fatal so catching it is not an option (AFAIK). I am planning to have the user elect to use the unaligned buffer and will not set it by default, that way the user can decide to take the performance hit or align the buffer if they chose.

So catching the error is definitely not an option since swift is actually throwing the fatal error. However with the verifier should've at least verified the buffer before hand.

If we could merge this change that would be great!

Yes running the CI now.

I have tried the verifier but it doesn't fail for me with the overall buffer but there is a header in the message buffer that fails afterward. When I have sometime I will investigate, as it will allow for a non fatal approach.

Would be perfect if you can provide an example, since we need to have the verifier actually work instead of just failing randomly.

Also, is it possible to get a new version released? It seems that the last version for the swift flatbuffers was prior to your last changes.

I can ask but no promises for the near future.

mustiikhalil

LGTM

abandy · 2023-10-27T00:11:17Z

@mustiikhalil I have a follow up PR in arrow after this has been merged. Afterward, I will create a fork using the verifier and post the instructions on how to repro an error here.

mustiikhalil · 2023-10-27T00:14:12Z

@abandy sounds great! seems that there is some cpp code that's failing (not sure why though). I also fixed the verifier failing in the following PR since it seemed that we had an issue with verifying arrays that contained forwardoffset's

mustiikhalil · 2023-10-27T00:25:07Z

I have a follow up PR in arrow after this has been merged.

If the changes aren't big, just add them within this PR and we can rename it/tag the new issue.

abandy · 2023-10-27T11:20:24Z

I have a follow up PR in arrow after this has been merged.

If the changes aren't big, just add them within this PR and we can rename it/tag the new issue.

My follow up PR is in the Apache Arrow repo.

abandy · 2023-10-27T11:21:50Z

@abandy sounds great! seems that there is some cpp code that's failing (not sure why though). I also fixed the verifier failing in the following PR since it seemed that we had an issue with verifying arrays that contained forwardoffset's

Awesome, that is great, thank you!

This change shouldn't affect the C++ tests. I noticed your PR is failing in on the same C++ test as well.

abandy · 2023-10-30T14:38:39Z

@mustiikhalil Is there a way to force the failing test to run again?

mustiikhalil · 2023-10-30T15:19:01Z

@abandy ~~Done~~ Seems like its still failing

abandy · 2023-10-30T20:00:34Z

@abandy ~~Done~~ Seems like its still failing

Agreed, strange, all the other C++ versions passed. Is there a flatbuffers dev who manages the C++ versions that can take a look at this?

abandy · 2023-10-31T12:58:11Z

@mustiikhalil I hope all is well. The C++ error shouldn't be related to these swift changes. Do you know if there is someone we can reach out to in order to request assistance in resolving the C++ 23 linux issue?

mustiikhalil · 2023-10-31T14:52:06Z

@CasperN should we just merge since the error is more of a cpp compile error?

CasperN · 2023-11-01T14:33:46Z

Looks like CI is trying to build flatc with CPP23. I agree that issue should predate this CI, but this seems more like a @dbaileychess question. Do you know which PR might have triggered it?

mustiikhalil · 2023-11-01T20:57:51Z

~~I have no clue, it seems that it simply stopped working one day~~ It seems that this PR is the pr that started the issue actually.

abandy · 2023-11-01T23:15:45Z

Agreed, it looks like the issue started when CI ran against this PR and is happening for subsequent PRs as well. Maybe something changed in the CI (a change in the docker image)?

abandy · 2023-11-06T12:48:07Z

@mustiikhalil @CasperN any chance we can get this issue resolved soon?

mustiikhalil · 2023-11-07T13:48:23Z

Personally, i dont mind merging this. Since the changes are only related to swift code, However would love to get a second approval to this decision. And the CI issue seems to be the algorithms lib itself, so there is a chance its not an issue we can solve but rather something Github actions should.

kou · 2023-11-09T00:15:32Z

@mustiikhalil How about #8151 for the C++ CI failure?
Here is a CI result on my fork: https://github.com/kou/flatbuffers/actions/runs/6805356298

mustiikhalil · 2023-11-12T10:44:36Z

@kou unfortunately I'm not responsible for cpp, however I already tagged someone who is! hopefully this would be resolved asap

mustiikhalil · 2023-11-18T08:30:12Z

@abandy can you rebase so we can get this PR merged

mustiikhalil · 2023-11-20T13:23:44Z

@abandy all good, should i merged? or do you have any extra changes?

abandy · 2023-11-20T22:33:07Z

@abandy all good, should i merged? or do you have any extra changes?

@mustiikhalil please merge when you get a chance. Thank you!

…ods (google#8134)

It includes google/flatbuffers#8134 .

### Rationale for this change flatbuffers v24.3.7 includes google/flatbuffers#8134 . The current master has a build error: https://github.com/apache/arrow/actions/runs/11357784776/job/31591213976?pr=44431#step:5:1114 ```text [51/77] Compiling Arrow ArrowType.swift /arrow/swift/Arrow/Sources/Arrow/File_generated.swift:107:206: error: value of type 'Table' has no member 'postion' public var schema: org_apache_arrow_flatbuf_Schema? { let o = _accessor.offset(VTOFFSET.schema.v); return o == 0 ? nil : org_apache_arrow_flatbuf_Schema(_accessor.bb, o: _accessor.indirect(o + _accessor.postion)) } ~~~~~~~~~ ^~~~~~~ ``` ### What changes are included in this PR? Use v24.3.7 instead of master. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #44432 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

…ods (google#8134)

github-actions bot added the swift label Oct 26, 2023

abandy mentioned this pull request Oct 26, 2023

[Swift][FlightRPC] Processing valid utf-8 lead to misaligned error apache/arrow#37884

Closed

abandy force-pushed the 8133 branch from 9341db6 to fc533ba Compare October 26, 2023 14:26

mustiikhalil approved these changes Oct 26, 2023

View reviewed changes

mustiikhalil mentioned this pull request Oct 27, 2023

[Swift] Fixes forward offset verifiable objects within arrays & fixes failing tests #8135

Merged

[Swift] Add allowReadingUnalignedBuffers to most ByteBuffer init methods

df6ad71

abandy force-pushed the 8133 branch from fc533ba to df6ad71 Compare November 20, 2023 12:20

Merge branch 'master' into 8133

84a2394

mustiikhalil enabled auto-merge (squash) November 20, 2023 22:47

mustiikhalil merged commit 5a937f1 into google:master Nov 20, 2023
48 checks passed

candysonya pushed a commit to candysonya/flatbuffers that referenced this pull request Jan 8, 2024

[Swift] Add allowReadingUnalignedBuffers to most ByteBuffer init meth…

b947264

…ods (google#8134)

BrewTestBot mentioned this pull request Mar 8, 2024

flatbuffers 24.3.7 Homebrew/homebrew-core#165459

Merged

kou added a commit to kou/arrow that referenced this pull request Oct 16, 2024

apacheGH-44432: [Swift] Use flatbuffers v24.3.7 or later

6983ebb

It includes google/flatbuffers#8134 .

kou mentioned this pull request Oct 16, 2024

GH-44432: [Swift] Use flatbuffers v24.3.7 apache/arrow#44433

Merged

jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024

[Swift] Add allowReadingUnalignedBuffers to most ByteBuffer init meth…

e96c4e7

…ods (google#8134)

jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024

[Swift] Add allowReadingUnalignedBuffers to most ByteBuffer init meth…

0e46484

…ods (google#8134)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Swift] Add allowReadingUnalignedBuffers to most ByteBuffer init methods #8134

[Swift] Add allowReadingUnalignedBuffers to most ByteBuffer init methods #8134

abandy commented Oct 26, 2023 •

edited

Loading

google-cla bot commented Oct 26, 2023

mustiikhalil commented Oct 26, 2023

abandy commented Oct 26, 2023

mustiikhalil commented Oct 26, 2023 •

edited

Loading

abandy commented Oct 26, 2023

mustiikhalil commented Oct 26, 2023

mustiikhalil left a comment

abandy commented Oct 27, 2023

mustiikhalil commented Oct 27, 2023

mustiikhalil commented Oct 27, 2023

abandy commented Oct 27, 2023

abandy commented Oct 27, 2023 •

edited

Loading

abandy commented Oct 30, 2023

mustiikhalil commented Oct 30, 2023 •

edited

Loading

abandy commented Oct 30, 2023

abandy commented Oct 31, 2023

mustiikhalil commented Oct 31, 2023

CasperN commented Nov 1, 2023

mustiikhalil commented Nov 1, 2023 •

edited

Loading

abandy commented Nov 1, 2023

abandy commented Nov 6, 2023

mustiikhalil commented Nov 7, 2023

kou commented Nov 9, 2023

mustiikhalil commented Nov 12, 2023

mustiikhalil commented Nov 18, 2023

mustiikhalil commented Nov 20, 2023

abandy commented Nov 20, 2023

[Swift] Add allowReadingUnalignedBuffers to most ByteBuffer init methods #8134

[Swift] Add allowReadingUnalignedBuffers to most ByteBuffer init methods #8134

Conversation

abandy commented Oct 26, 2023 • edited Loading

google-cla bot commented Oct 26, 2023

mustiikhalil commented Oct 26, 2023

abandy commented Oct 26, 2023

mustiikhalil commented Oct 26, 2023 • edited Loading

abandy commented Oct 26, 2023

mustiikhalil commented Oct 26, 2023

mustiikhalil left a comment

Choose a reason for hiding this comment

abandy commented Oct 27, 2023

mustiikhalil commented Oct 27, 2023

mustiikhalil commented Oct 27, 2023

abandy commented Oct 27, 2023

abandy commented Oct 27, 2023 • edited Loading

abandy commented Oct 30, 2023

mustiikhalil commented Oct 30, 2023 • edited Loading

abandy commented Oct 30, 2023

abandy commented Oct 31, 2023

mustiikhalil commented Oct 31, 2023

CasperN commented Nov 1, 2023

mustiikhalil commented Nov 1, 2023 • edited Loading

abandy commented Nov 1, 2023

abandy commented Nov 6, 2023

mustiikhalil commented Nov 7, 2023

kou commented Nov 9, 2023

mustiikhalil commented Nov 12, 2023

mustiikhalil commented Nov 18, 2023

mustiikhalil commented Nov 20, 2023

abandy commented Nov 20, 2023

abandy commented Oct 26, 2023 •

edited

Loading

mustiikhalil commented Oct 26, 2023 •

edited

Loading

abandy commented Oct 27, 2023 •

edited

Loading

mustiikhalil commented Oct 30, 2023 •

edited

Loading

mustiikhalil commented Nov 1, 2023 •

edited

Loading