search

apache / druid

Apache Druid: a high performance real-time analytics database.
https://druid.apache.org/
Apache License 2.0
13.49k stars 3.7k forks source link

IncrementalIndex's `in` ThreadLocal doesn't need to be instance-level #8886

Open leventov opened 4 years ago

leventov commented 4 years ago

IncrementalIndex's ThreadLocal in can be static final. This ThreadLocal doesn't store anything specific to a given IncrementalIndex: rather, it's used to set a Row temporarily, and there is no situation when an IncrementalIndex could call recursively into another IncrementalIndex, so this temporary assignment cannot be disturbed.

As a safety net, we could add a check if (in.get() != null) throw ISE(..); in.set(row);

Furthermore, it would be ideal to get rid of ThreadLocal altogether (which would be beneficial for performance, because every Aggregator wouldn't need to query ThreadLocal upon every indexed row (ThreadLocal.get() is like Map.get(), not super cheap), and for reducing complexity, but this requires a significant refactoring of Aggregator interface and/or indexing logic, so saving this idea for a better time.

sbespalov commented 4 years ago

@leventov can you please have a look at PR?