Cesium icon indicating copy to clipboard operation
Cesium copied to clipboard

Address perf+correctness of StrLen and StrChr

Open neon-sunset opened this issue 1 year ago • 5 comments

neon-sunset avatar Nov 17 '23 11:11 neon-sunset

Aha, we get AVE. Let me check. Probably need to account for the fact that .IndexOf thinks it owns the memory when it doesn't causing it to dereference a memory range past null byte on vector load at a memory page boundary where we don't have read access to the next page, causing segfault.

neon-sunset avatar Nov 17 '23 18:11 neon-sunset

@ForNeVeR In this particular case it's not an issue at all, to support NS2.0. It might, however, become an issue in the future.

A few thoughts: I don't think trying to reimplement stdlib is a scalable approach - it will either take too much effort or the quality of the resulting code may be insufficient.

Ideally, we should be mixing and matching the following options on a case-by-case basis:

  • Forwarding calls to .NET CoreLib
  • P/Invoking host-provided stdlib, like .NET itself does (e.g. it will call memcpy above certain threshold, even though today its own implementation literally does a better job than most due to conservative compilation flags of your average glibc)
  • Handling happy-path in C# and then deferring to either of the above

P/Invokes are already very cheap/almost free in .NET (Жова and Go could never) but in the future, if it ever becomes a concern, they can be made as cheap as calls in an actual compiled C code by applying [SuppressGCTransition], generating DirectPInvokes for NativeAOT and caching function pointers to a readonly static for JIT (if it doesn't do it already).

UPD: After further discussion with Andrii I might have been too hasty on this, the point stands but things like C locales are scary 😅

neon-sunset avatar Nov 17 '23 21:11 neon-sunset

@ForNeVeR The issue with SEGFAULT should be addressed now - I opted into a simpler approach with aligning the pointer at 16B boundary first and then using 128b vectors for searching the matches directly. This way it should never cross a page boundary (which was causing issues).

neon-sunset avatar Dec 30 '23 12:12 neon-sunset

I had to bump up .Runtime to net7.0 because cross-platform Vector API is not available on older targets. Let me know if it's an issue.

neon-sunset avatar Dec 30 '23 15:12 neon-sunset

I am ok with the implementation, but would insist on having a fallback for older runtimes. Do you want to implement it yourself, or will entrust it to me?

Alright, I will if-def it. "Proper" way to fix it is by copying impl. from Glibc with its bitwise tricks (which we, in .NET, do not need) but I guess "works on NETFW, if you care about performance that's on you" is also fine.

neon-sunset avatar Jan 04 '24 19:01 neon-sunset