search

eclipse / omr

Eclipse OMR™ Cross platform components for building reliable, high performance language runtimes
http://www.eclipse.org/omr
Other
940 stars 394 forks source link

Define a lifecycle of client-provided data in JitBuilder API #4513

Open janvrany opened 4 years ago

janvrany commented 4 years ago

When using (some of) JitBuilder APIs, client passes down data that do not originate in JitBuilder. For example, when defining a parameter like DefineParameter(name, Int32), string name is allocated outside JitBuilder.

It is not clear what's the lifecycle of such data (whether or not passing this data to ani API transfers the ownership and if not, when it is safe to free it).

The aim of this issue is to (i) discuss what is the desired lifecycle (ii) document it and, if neccessary (iii) fix JitBuilder API to match (i).

As of now, no ownership is transferred and the data must remain valid while compileMethodBuilder() is executing.

This issue is a result slack conversation https://app.slack.com/client/T32QAFE48/CBNMC7UJY

janvrany commented 4 years ago

To kick-off the discussion, my preference as of JitBuilder user is that no ownership is transferred and the JitBuilder APIs make copies of all user-originated data (if they're required after returning from the API call). In most cases "data" means strings, not big, so having simpler and more robust API for the price of slightly increased memory consumption is a good deal.

mstoodle commented 4 years ago

Thanks for opening this issue to capture our discussion from the slack channel.

I agree with @janvrany . There are really two lifetimes involved imo : 1) the lifetime of the MethodBuilder object, and 2) the lifetime of compileMethodBuilder().

The strings passed into services in the user's MethodBuilder constructor should be copied and "registered" against the MethodBuilder's lifetime (so they are freed when the MethodBuilder object is destructed).

Strings passed into the services used inside buildIL() should be copied and "registered" against the lifetime of compileMethodBuilder() meaning that, near the end of compileMethodBuilder() a call should be made to the compiled MethodBuilder object to free these strings. I believe there is already a call like that in compileMethodBuilder() to clean up dangling SymbolReference pointers that are created by the OMR compiler and stored on the MethodBuilder so I don't actually think it would be a lot of work to implement this scheme.

Of course, you could get away with only the first, but the second lifetime could be substantially shorter than the first one, so I think it's also worthwhile given I think the implementation cost would be pretty low.

janvrany commented 4 years ago

Beside strings passed to various APIs, a lifecycle of MethodBuilders used to inline methods / functions should be defined & documented.

Is it sufficient to cleanup such MethodBuilder instances in top-level MethodBuilder dtor?

mstoodle commented 4 years ago

Is it sufficient to cleanup such MethodBuilder instances in top-level MethodBuilder dtor?

Probably, yes, but it may also be possible to clean them up on the way back from the compileMethodBuilder() call (if they were allocated during buildIL() )