gopher-lua icon indicating copy to clipboard operation
gopher-lua copied to clipboard

Refactor LTable to improve performance

Open shawnclovie opened this issue 4 years ago • 3 comments

Improve conversion speed between lua value and go value by reduce memory allocation。

Changes proposed in this pull request:

append lTableDictValue{value LValue, keyIndex int}. change LTable would contains unique dict map[LValue]lTableDictValue. change LTable#keys as []*LValue to reduce memory allocation on RawSetString. append LTable#DictionaryLen() (use to initial go map cap). append some convert functions between LValue and go value.

Benchmark testing:

  • Load the 28kb json decode as map[string]interface{}, convert to LTable and then convert back.
  • original BenchmarkToLValue-4 2157 559233 ns/op 272908 B/op 3799 allocs/op BenchmarkToGValue-4 3924 270173 ns/op 126799 B/op 2025 allocs/op
  • changed BenchmarkToLValue-4 2704 377055 ns/op 187883 B/op 3459 allocs/op BenchmarkToGValue-4 6284 169170 ns/op 82971 B/op 981 allocs/op

I would charge test code, if the changing would be accepted.

shawnclovie avatar May 26 '20 09:05 shawnclovie

Coverage Status

Coverage decreased (-0.8%) to 87.286% when pulling dbe16f64515a2b4960708fb1d5954874d7c1d609 on shawnclovie:refactor/improve_pref into 6ff375d91eabb95a1fb05873aef638c2583f3704 on yuin:master.

coveralls avatar May 26 '20 09:05 coveralls

current table implementation is even not correct in some cases, what about reviewing #283

chyh1990 avatar Jun 01 '20 13:06 chyh1990

I think that maybe this change:

" change LTable#keys as []*LValue to reduce memory allocation on RawSetString."

May not be doing what's wanted.

LValue is an interface type, and as such is modelled behind the scenes as:

type iface struct {
        tab  *itab
        data unsafe.Pointer
}

So a slice of LValues []LValue is modelled in memory as:

*itab
unsafe.Pointer
*itab
unsafe.Pointer
*itab
*unsafe.Pointer
....

I think changing the type to be []*LValue will actually change the slice to be an array of pointers to new allocated iface structures on the heap, which will then point to the actual value. i.e.

*iface
*iface
*iface
....

These iface elements will be heap allocated. In other words, it will introduce an additional redirection and also potentially spread the values around the heap so that linear iteration of the slice may potentially incur additional cache misses.

I did an investigation of slices of interface{} a couple of years back and found it to be like this, I did a write up here: https://tul.github.io/2018/07/23/go-interfaces-deep-dive.html

So I actually think this particular aspect of the change may be detrimental to performance.

tul avatar Dec 10 '20 20:12 tul