Truffle API feedback

[Michael Van De Vanter]

Hi Tim,

Yes, likewise.  I’ll attend to these, but maybe not right away because of other 
priorities right now - mainly preparing two presentations for ECOOP.

	mlvdv

> On Jun 19, 2015, at 11:20 AM, christian.humer at gmail.com wrote:
> 
> Hi Tim,
> 
> Thanks for your feedback. We really appreciate this.
> 
> Ad 1,4) I will leave these for Jaroslav as I think he needs to revamp the options system for multi tenancy anyway. TruffleOptions is indeed not a nice solution and I hope we can integrate it into a new options system.
> 
> Ad 2) In general I also like these kind of abstractions to avoid mistakes but I think in this case we should rather not box the probability value into an object. It would give us very little benefit and quite some complexity in the Truffle/Graal intrinisc implementation. Besides that, most guest languages are not using it directly. Instead they use ConditionProfile#createCountingProfile().
> 
> Ad 3) I agree. Will fix it for ReplaceObserver. Michael Van De Vanter can you fix this for SourceListener?
> 
> Ad 5,6) I will leave these for Michael Van De Vanter as well.
> 
> Please don't hesitate to shoot with more feedback at us :-)
> 
> 
> - Christian Humer
> 
> 
> 
> ------ Original Message ------
> From: "Tim Boudreau" <niftiness at gmail.com>
> To: graal-dev at openjdk.java.net
> Sent: 19.06.2015 18:59:16
> Subject: Truffle API feedback
> 
>> Hi, folks,
>> 
>> I've been poking around randomly in the Truffle sources, and made a few
>> notes on places where the API could be cleaner (which I think Jarda Tulach
>> is actively working on):
>> 
>> 1a.  CompilerOptions.setOption(String name, Object value);
>> 
>> would be easier to use if it returned CompilerOptions instead of void, so
>> method chaining can be used.  In general, any time something like this
>> returns void, you're throwing away an opportunity to make calling code less
>> verbose.
>> 
>> 1b. A cleaner and safer pattern would be:
>> 
>> public abstract class CompilerOption<T> {
>>   protected abstract void validate(T value) throws SomeException;
>> //because you can
>> }
>> 
>> and
>> 
>> <T> CompilerOptions.setOption(CompilerOption<T>, T value);
>> 
>> and let people define static fields with CompilerOption instances.
>> 
>> That would be typesafe and would eliminate typos in string names.  And
>> would probably allow language authors to factor their option processing
>> more cleanly than a big switch statement.
>> 
>> --------
>> 
>> 2. CompilerDirectives.injectBranchProbability(double probability, boolean
>> condition) and the constants such as LIKELY_PROBABILITY
>> 
>> You're better off with a Probability class that hides the floating point.
>> There's no particular reason this sort of computation needs to be floating
>> point, and if it does, you're still better off with
>> Probability.isMoreLikelyThan(otherProbability) or
>> Probability.times(otherProbability).
>> 
>> More flexibility later, you can put bounds on legal values (what would a
>> probability of Float.MIN_VALUE mean?  Make that impossible), and it's
>> cleaner.
>> 
>> --------
>> 
>> 3.  ReplaceObserver.nodeReplaced - observer/listener methods are clearer if
>> named on$EVENT, e.g. onNodeReplaced - so if implemented as a mix-in, it is
>> still obvious that this method responds to an event.
>> 
>> Same for SourceListener, and probably other things named *Listener /
>> *Observer.
>> 
>> --------
>> 
>> 4.  Should TruffleOptions really have world-writable static fields?
>> 
>> --------
>> 
>> 5.  BytesDecoder.decode() - should probably return CharSequence, not
>> String.  With CharSequence you keep the option of avoiding a memory copy,
>> with an implementation of CharSequence over the raw array bytes.
>> 
>> Once it's String, you can never go back.  Same for Source.getCode().
>> 
>> --------
>> 
>> 6.  Source.setFileCaching() - this does not look like it should be API.
>> Maybe some attribute of whatever is using the Graal API on initialization
>> (perhaps user code -> language -> truffle), but not here.
>> 
>> --
>> http://timboudreau.com
>