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

Replace all runtime platform checks with linktime conditions #3335

Merged
merged 2 commits into from Jun 22, 2023

Conversation

armanbilge
Copy link
Member

No reason to pay a performance penalty at runtime when we can elide the useless code at linking time.

@@ -836,8 +836,10 @@ object File {

// found an absolute path. continue from there.
case link if link(0) == separatorChar =>
if (Platform.isWindows() && strncmp(link, c"\\\\?\\", 4.toUInt) == 0)
path
if (isWindows)
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be possible to remove the include of Platform.
I was moving quickly, but I did not see any other uses of it in the this new file.

@LeeTibbert
Copy link
Contributor

This PR is a public service, thank you. It eliminates some artifacts of Evolution.

I looked at the network related files. A quick scan for concept, not character by character.
All looked good.

I did not see any edits to Platform.scala. Should the isLinux etc methods there be removed altogether?

@armanbilge
Copy link
Member Author

armanbilge commented Jun 15, 2023

Thank you for the review!

I did not see any edits to Platform.scala. Should the isLinux etc methods there be removed altogether?

This is a good question. These are public APIs and they are implemented slightly differently than LinktimeInfo. However I am not sure in which situations they would be useful now, if at all. I'd be happy to see them removed to avoid having too many ways to do similar things.

@LeeTibbert
Copy link
Contributor

I think the APIs are public in the sense of "having been in SN variants released to the wild" not public
in the "This is in Java or ScalaJVM". If I am wrong and the latter is true, the story changes.
V5.0 is the time to break API, especially those with which we will have to live for awhile.

Please double check my thinking, too easy to throw the baby out with the bath water.
Someone starting a process on one OS and migrating it to another OS might have a situation
where the Runtime runtime hardware architecture differs from the linktime architecture.

There might be C code backing the Platform isLinux etc. methods. That can probably
be deleted also.

I'd be happy to see them removed to avoid having too many ways to do similar things.

Yes, I myself have been left pondering when to use Platform and when to use LinkTimeInfo and
getting tangled up. Probably why you see Platform in the network code.

I am a big fan of the Python "one way to do things" concept.

@armanbilge
Copy link
Member Author

I think the APIs are public in the sense of "having been in SN variants released to the wild"

This is correct, these are Native lib APIs that are not private to Scala Native.


Someone starting a process on one OS and migrating it to another OS might have a situation
where the Runtime runtime hardware architecture differs from the linktime architecture.

Well, there is some nuance here 😅

I described the Platform checks as having a runtime penalty. What I meant by this, is that they compile to a function call. This is different from LinktimeInfo checks which are constants that exist only at linking-time and are used to eagerly perform dead-code-elimination of branches in the logic.

However, even though the Platform checks are function calls, what they return may be compile-time constants.

int scalanative_platform_is_freebsd() {
#if defined(__FreeBSD__)
return 1;
#else
return 0;
#endif
}
int scalanative_platform_is_linux() {
#ifdef __linux__
return 1;
#else
return 0;
#endif
}
int scalanative_platform_is_mac() {
#ifdef __APPLE__
return 1;
#else
return 0;
#endif
}

Although in other instances they may also do runtime checks e.g.

int scalanative_platform_probe_mac_x8664_is_arm64() {
int translated = 0;
#ifdef __APPLE__
size_t translatedLen = sizeof(translated);
int ret = sysctlbyname("sysctl.proc_translated", &translated,
&translatedLen, NULL, 0);

In any case, at least for the current checks we have in Platform, I cannot imagine a viable program where the check would evaluate differently at compile/linking-time and runtime.

@LeeTibbert
Copy link
Contributor

OK, I think I have found a counter example which argues for keeping Platform, probably with
a comment recommending using LinkTimeInfo equivalent.

printf(s"Running on Linux: ${Platform.isLinux()}\n That is, the isFoo() occurs outside an if,
Yes, the code can be converted by adding a `if (LinkTimeInfo.isLinux) printf ("Running on Linux\n")

At first blush, it looks like some Platform.c code could be eliminated and Platform.scala
converted to use if (ifLinkTimeInfo.isFoo) true else false. However the Platform object is extern`. I at one point there was some work to remove the restriction that all methods
in an extern object needed to be extern. I do not know if that work has ever been merged.

So, leaving the .c code, possibly with a comment in Platform.scala might defer diving
into a time suck with poor benefit/cost ratio.

@LeeTibbert
Copy link
Contributor

Upon further investigation: Rationalizing/unifying Platform is probably a task for the next person who
messes up badly. There are various "Platform.scalas. The ones in scripted-testsrefer back to theruntime` Platform (which I had proposed to eliminate). Some of the Platforms use direct Java os properties
(probably for good reason).

I hereby withdraw my proposal to rationalize Platform and pray that I am not the next one to mess up
badly enough to get thrown into that pit of misery.

@WojciechMazur
Copy link
Contributor

You can rebase it on top of #3337 when it's merge to get rid of failing tests in tools/test

@armanbilge armanbilge marked this pull request as ready for review June 15, 2023 15:00
@WojciechMazur WojciechMazur merged commit 5820225 into scala-native:main Jun 22, 2023
77 checks passed
WojciechMazur added a commit to WojciechMazur/scala-native that referenced this pull request Sep 1, 2023
WojciechMazur added a commit that referenced this pull request Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants