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

Speed up StringTools.(starts|ends)With on Lua #10983

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

sebthom
Copy link
Contributor

@sebthom sebthom commented Feb 28, 2023

With this PR applied StringTools.startsWith is 10 times faster and StringTools.endsWith is 6 times faster on Lua.

It is also faster than using lua.NativeStringTools.sub which instantiates - in this case unnecessary - temporary @:multiReturn objects.

@Simn
Copy link
Member

Simn commented Feb 28, 2023

Shouldn't we have a way of calling this sub-function without using untyped? I thought this was what NativeStringTools.sub did, but I guess there's something else going on.

@sebthom
Copy link
Contributor Author

sebthom commented Feb 28, 2023

As I mentioned in the description, using the NativeStringTools comes with a performance penalty (I suspect because of the emulated @:multiReturn handling). In my tests the overhead was up to 100%.

@Simn
Copy link
Member

Simn commented Feb 28, 2023

Yes I read that this is your observation, my question is why that is the case. If using untyped __lua__ is superior to a properly typed extern then we're having a larger problem to consider.

But I'm not very familiar with lua, so I can't really estimate the effort to address this "properly". If you say that this is at least a pragmatic approach then I'm happy to merge it.

@sebthom
Copy link
Contributor Author

sebthom commented Feb 28, 2023

I cannot really answer the question why it is faster using the untyped version.

Anyways, I replaced it with NativeStringTools.sub for now to avoid any confusion. This still is a win as it also is considerably (9 times) faster than the current implementation.

@sebthom sebthom force-pushed the patch-5 branch 2 times, most recently from f92dad0 to 3172788 Compare March 1, 2023 09:00
@Simn
Copy link
Member

Simn commented Mar 1, 2023

I'm seeing this:

results: SOME TESTS FAILURES (success: false)
unit.issues.Issue8211
  test: FAILURE .FF
    line: 6, expected true
    line: 7, expected true

@sebthom
Copy link
Contributor Author

sebthom commented Mar 1, 2023

I will look into it

@skial skial mentioned this pull request Mar 1, 2023
1 task
@sebthom
Copy link
Contributor Author

sebthom commented Mar 1, 2023

apparently it does not work when using NativeStringTools.sub because we pass searchString.length to it which is not unicode aware. However bypassing NativeStringTools and relying on Lua to directly figure out the length of the search string using the lua operator # works. So one more reason to use the first approach. It is even faster and also works with unicode strings. I am going to change the PR again

@sebthom
Copy link
Contributor Author

sebthom commented Mar 1, 2023

@Simn, tests are green now.

@Simn
Copy link
Member

Simn commented Mar 1, 2023

Hmpf, I'm very reluctant to add something involving untyped to std in 2023... At the very least we should have something like Syntax.code for lua, but we can look into that as part of another issue.

@Simn Simn merged commit eb57aef into HaxeFoundation:development Mar 1, 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.

2 participants