ntt icon indicating copy to clipboard operation
ntt copied to clipboard

Add cache for binary search in lines-map

Open 5nord opened this issue 3 years ago • 6 comments

The method syntax.Node.Span uses binary search to convert a file offset into a line number. Location heavy tools, like our language server or formatter, would benefit from line-caches. For example like so:

func (t *tree) position(pos int) Position {
    if t.cachedLineBegin <= pos && pos < t.cachedLineEnd {
        line = t.cachedLine
    }
   // ...
}

To be efficient we probably need two separate line-caches, one for n.Pos() and one for n.End(). ~~Best would be creating benchmarks comparing random position access, accessing only begin-offsets and accessing whole spans (begin- and end-offset).~~

5nord avatar Oct 02 '22 11:10 5nord

Added benchmarks in https://github.com/nokia/ntt/pull/646

@nitinNT: Would this maybe interest you?

5nord avatar Oct 08 '22 14:10 5nord

yes, i will try

nitinNT avatar Oct 10 '22 18:10 nitinNT

@5nord : i have started working on this issue and i have noted the things which i need TODO and here are things

  • modify tree struct present in syntax.go by adding fields (cachedLineBegin ,cachedLineEnd,cachedLine) and all of them will be int.
  • modify searchLines function in syntax.go and modify newly added fields mentioned in above step according to binary search.

am i correct in above steps?

nitinNT avatar Oct 17 '22 20:10 nitinNT

Sounds good to me.

What you could do is executing the benchmarks before and after your change to see how big the improvement is.

5nord avatar Oct 17 '22 20:10 5nord

@5nord i have tried the above approach but it seems its not improving the performance. can you little guide me?

nitinNT avatar Oct 29 '22 20:10 nitinNT

No worries. It happens sometimes that an idea is not as good as we initially thought. That's why we measure first. And a negative result can still be a valuable result 🙂

Sure I can help you. I suggest you push your PR as a draft and we'll have a look.

5nord avatar Oct 29 '22 20:10 5nord