sdk icon indicating copy to clipboard operation
sdk copied to clipboard

[vm/ffi] Performance cliff for nested structs and arrays if used both with `Pointer` and `TypedData` backing store

Open dcharkes opened this issue 1 year ago • 0 comments

https://github.com/dart-lang/sdk/blob/ab09f99638af8b50c2b8700cd6bd07b9e89f6337/pkg/vm/lib/transformations/ffi/common.dart#L913-L928

This pattern of checking for whether something is backed by a Pointer, and doing pointer arithmetic if it is a pointer and a typed data view if it is not a pointer causes a performance cliff when TFA cannot prove that one of the branches is definitely not happening.

https://github.com/dart-lang/sdk/commit/ddfc00e7739108a7e29092ec4559fbd74733f2bb caused benchmarks/FfiStructCopy/dart/FfiStructCopy.dart to slow down 15x. This is because the TypedData path in the if is no longer removed by TFA (due to the TypedData constructors for Array now being shared). This causes the inliner to bail out on inlining all Pointer and TypedData view calculations. Which in turn causes a bunch of calls and allocations.

I presume that any reasonably large FFI program would have both Pointer and TypedDatas backing structs, so this micro-benchmark was probably giving us a too rosy view. (Because if this, I don't believe a revert is in order.)

We should try to remove any branching on something being a Pointer or TypedData.

Some ideas:

  • a. Making a recognized method that does a typedDataBase.offsetBy and calling that in the struct/array implementation instead of branching on an instanceOf<Pointer>. The question is what IL/machine code to generate in this case. We'd still need to branch on whether to allocate a Pointer object or an TypedDataView object.
  • b. Make Structs/Arrays always work on external typed data and never use Pointers.
  • c. Have an offset field in _Compound and never create new typed data views or pointers at all. Nested struct and array accessors would return the same typed data base object but with a different int offset.

cc @mkustermann I believe you have argued for option (c) before.

dcharkes avatar Feb 12 '24 13:02 dcharkes