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

Add scala native support #730

Merged
merged 9 commits into from
Dec 5, 2024
Merged

Add scala native support #730

merged 9 commits into from
Dec 5, 2024

Conversation

PhoenixmitX
Copy link
Contributor

@PhoenixmitX PhoenixmitX commented Nov 22, 2024 •
edited
Loading

Attempt to add scala native support

TODOs:

  • Update documentation
  • Run tests with LITTLE_ENDIAN
  • Run tests with BIG_ENDIAN
  • Maybe some manual testing
  • Update pipelines

closes #633

@PhoenixmitX PhoenixmitX marked this pull request as ready for review November 23, 2024 01:06
@PhoenixmitX
Copy link
Contributor Author

PhoenixmitX commented Nov 23, 2024 •
edited
Loading

@sirthias
This should be done so far.
The review may be easier if you compare the Unsafe.scala file with the Unsafe.scala file from the jvm implementation.

I made a small local test to ensure that at least the basics work - for the rest i trust the existing automated tests.

I also updated the ci.yml:

  • bumped versions
  • tests were only running on jvm - now they run on js, jvm & native.

Also i commented the macrolizer dependencie in out, since it has no Scala Native support.
I also added metals-specific exclusions to the .gitignore as its the build server behind AFAIK every IDE except Intellij

I have no device running with BIG_ENDIAN, maybe someone should execute the tests with a device running on BIG_ENDIAN.

@PhoenixmitX
Copy link
Contributor Author

PhoenixmitX commented Nov 23, 2024 •
edited
Loading

In order to compile the native modules you may need to install the required dependencies for Scala Native on your machine.

@PhoenixmitX PhoenixmitX marked this pull request as draft November 24, 2024 13:15
@PhoenixmitX
Copy link
Contributor Author

I will make some changes to the Unsafe.scala file to call more low-level functions and possibly improve performance.

@PhoenixmitX
Copy link
Contributor Author

I looked at the reference provided by @plokhotnyuk in #633 (comment).
There they use llvm.bswap.i32 as a replacement for JInt.reverseBytes, llvm.bswap.i64 for JLong.reverseBytes and so on.
At first I wanted to use the direct calls too, but it turned out that JInt.reveseBytes is an inline def that calls llvm.bswap.i32 (see https://github.com/scala-native/scala-native/blob/main/javalib/src/main/scala/java/lang/Integer.scala#L349).
So I didn't change the native code implementation to be as close as possible to the Java implementation.

The only change I made was to use a memcpy method provided by scalanative instead of defining one.

@PhoenixmitX PhoenixmitX marked this pull request as ready for review November 24, 2024 15:54
@sirthias
Copy link
Owner

Thank you, @PhoenixmitX, for your contribution here and sorry for not chiming in earlier.
I was travelling these last days (and will be until Wednesday), with only limited connectivity.
I'll take a look at this PR as soon as I'm back at my desk with a little capacity to spare.
👍

sirthias
sirthias reviewed Nov 27, 2024
View reviewed changes
core/.native/src/main/scala/io/bullet/borer/internal/Unsafe.scala Outdated
def setOctaByteBigEndian(byteArray: Array[Byte], ix: Int, value: Long): Unit =
_setOctaByteBigEndian(byteArray, ix, JLong.reverseBytes(value))

final class BigEndianByteArrayAccess extends UnsafeByteArrayAccess(ByteOrder.LITTLE_ENDIAN):
Copy link
Owner

Choose a reason for hiding this comment

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

This should extend UnsafeByteArrayAccess(ByteOrder.BIG_ENDIAN), no?

Copy link
Contributor Author

@PhoenixmitX PhoenixmitX Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess you're correct, but I copied it from the Java implementation.
https://github.com/sirthias/borer/blob/master/core/.jvm/src/main/scala/io/bullet/borer/internal/Unsafe.scala#L293

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am on my way home.
I will look deeper into it when I arrive.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that's wrong there as well.
This bug probably never got triggered so far since "direct JSON parsing" is so far only available on little endian platforms. This should be improved...

Copy link
Owner

Choose a reason for hiding this comment

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

(not in this PR though!)

Copy link
Owner

Choose a reason for hiding this comment

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

I looked a bit more into this topic and, to be honest, big endian appears to be pretty much dead regarding the commercial importance. There are almost no relevant big-endian-only platforms produced anymore.
Everything out there is either directly little endian oder at least bi-endian, which ends up being configured to almost exclusively run in little-endian mode.

So, since borer should work fine on big endian systems as it is I'd rather spare the work for any big-endian optimizations. In all likelihood they simply won't ever see production use anywhere...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that mean that we leave the big endian support untested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If yes, what else needs to be done to get this merged?

Copy link
Owner

Choose a reason for hiding this comment

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

Does that mean that we leave the big endian support untested?

I've just created #731 for this.
Nothing big-endian related required in this PR.
👍

Copy link
Owner

Choose a reason for hiding this comment

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

If yes, what else needs to be done to get this merged?

Give me a bit more time to look at this PR and then I think we should be fine to merge.

@sirthias sirthias merged commit 97552e0 into sirthias:master Dec 5, 2024
12 of 13 checks passed
@sirthias
Copy link
Owner

sirthias commented Dec 5, 2024

Finally got around to properly looked at this.
This looks awesome, @PhoenixmitX, thank you very much again!

@PhoenixmitX
Copy link
Contributor Author

You're welcome 🤗 and thank you for merging this.

When can we expect a release including the Scala nativ Support?

@sirthias
Copy link
Owner

sirthias commented Dec 5, 2024

I'm in the process of adding one more little feature (JSON pretty printing) and then I'll cut the next release.
Hopefully I'll be done tomorrow...
(Always release on a Friday, right? ;-)

@sirthias
Copy link
Owner

sirthias commented Dec 6, 2024

Ok, I just published version 1.15.0 to sonatype, containing for scala native as well as pretty JSON rendering.
Thank you so much again, @PhoenixmitX, for this PR! ❤️

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

@2m 2m 2m left review comments

@sirthias sirthias sirthias left review comments

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

Successfully merging this pull request may close these issues.

An ode to borer, or: Please add support for scala-native
3 participants
@PhoenixmitX @sirthias @2m