FfiCallbackMetadata::kPageSize should be target::kOSPageSize or similar?

[eseidel]

https://github.com/dart-lang/sdk/blob/main/runtime/vm/ffi_callback_metadata.h#L200

We've ended up replicating this logic in other places (we use this thunk/stub trick in a few more places) and copying those constants. They have little to do with FFI and seem like they belong in some central location? They appear to represent the minimum page allocation size (rather than target::kPageSize which represents Heap's preferred allocation size).

I'm about to write a patch to move ours to page.h, but maybe they belong in runtime_api.h? In any case, mostly what I'm looking for is taste from @mraleph or @dcharkes. Happy to send patches up your way once we've done the move ourselves. Presumably target::kPageSize also should be renamed to target::kHeapPageSize, since it's specific to the Dart heap, not the target's default allocation size. This new constant would be target::kOSPageSize or similar?

[dcharkes]

Right, FfiCallbackMetadata::kPageSize

https://github.com/dart-lang/sdk/blob/bfe68f9ff552e3a11dcc218ee502feeee6299340/runtime/vm/ffi_callback_metadata.h#L205-L216

Could be reused for other things that require a memory block which is at least as large as a page on both host and target. Moving it to a place where it is reuseable and renaming it sgtm if there's a use for that.

target::kPageSize also should be renamed to target::kHeapPageSize

@rmacnak-google is the one to say something about that.

Maybe @rmacnak-google can also suggest a good name if we move FfiCallbackMetadata::kPageSize to be reusable (as a page-size that is at least as large as host and target).

fyi @liamappelbe

[eseidel]

For now I've also just made all our classes just depend on FfiCallbackMetadata::kPageSize, which is fine. You're welcome to close this if there is no use upstream, thanks.

[rmacnak-google]

target::kPageSize also should be renamed to target::kHeapPageSize

No, kHeapPageSize refers to the size of a normal Dart heap chunk. Parts of the GC assume these chunks are also aligned to this value. This value is bigger than the OS page size. The overloading of the term "page" is unfortunate, but common.

[eseidel]

I think the real footgun here is that there is a "global" dart::kPageSize (everything is in namespace dart) and then various classes intentionally shadow that kPageSize and end up with confusing (or sometimes wrong) code if they get the wrong size not understanding the shadow.

In any case, we've worked around this by just using FfiCallbackMetadata::kPageSize with our own alias that is named uniquely from kPageSize so there is no risk of getting the wrong one.

We've solved this for us for now, so please feel free to close if not useful to the wider project.

[a-siva]

@rmacnak-google is there any actionable item here in terms of moving FfiCallbackMetadata::kPageSize and making a host and target version of it ?