From 840df4a22978c3ccf214a594e7a012eb44e53947 Mon Sep 17 00:00:00 2001 From: Thomas Pelletier Date: Wed, 26 May 2021 18:47:00 -0400 Subject: [PATCH] seen tracker: use array for storage (#545) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit name old time/op new time/op delta UnmarshalDataset/config-32 81.2ms ± 3% 77.8ms ± 3% -4.25% (p=0.000 n=20+19) UnmarshalDataset/canada-32 104ms ± 5% 105ms ± 4% ~ (p=0.102 n=20+20) UnmarshalDataset/citm_catalog-32 57.5ms ± 5% 59.0ms ± 5% +2.54% (p=0.033 n=20+20) UnmarshalDataset/twitter-32 25.7ms ± 7% 28.1ms ± 5% +9.33% (p=0.000 n=20+20) UnmarshalDataset/code-32 305ms ± 6% 292ms ± 5% -4.29% (p=0.000 n=20+19) UnmarshalDataset/example-32 519µs ± 6% 522µs ± 5% ~ (p=0.659 n=20+20) UnmarshalSimple/struct-32 1.44µs ± 1% 1.17µs ± 6% -18.78% (p=0.000 n=14+20) UnmarshalSimple/map-32 2.30µs ± 4% 1.99µs ± 4% -13.65% (p=0.000 n=20+19) ReferenceFile/struct-32 44.1µs ± 4% 38.1µs ± 5% -13.61% (p=0.000 n=18+20) ReferenceFile/map-32 197µs ± 7% 189µs ± 5% -3.91% (p=0.000 n=20+20) HugoFrontMatter-32 45.9µs ± 6% 39.3µs ± 6% -14.46% (p=0.000 n=19+20) name old speed new speed delta UnmarshalDataset/config-32 12.9MB/s ± 3% 13.5MB/s ± 3% +4.42% (p=0.000 n=20+19) UnmarshalDataset/canada-32 21.1MB/s ± 5% 20.9MB/s ± 4% ~ (p=0.101 n=20+20) UnmarshalDataset/citm_catalog-32 9.72MB/s ± 6% 9.47MB/s ± 5% -2.53% (p=0.031 n=20+20) UnmarshalDataset/twitter-32 17.2MB/s ± 7% 15.8MB/s ± 5% -8.57% (p=0.000 n=20+20) UnmarshalDataset/code-32 8.81MB/s ± 7% 9.20MB/s ± 5% +4.47% (p=0.000 n=20+19) UnmarshalDataset/example-32 15.6MB/s ± 6% 15.5MB/s ± 5% ~ (p=0.644 n=20+20) UnmarshalSimple/struct-32 7.61MB/s ± 1% 9.39MB/s ± 7% +23.33% (p=0.000 n=15+20) UnmarshalSimple/map-32 4.78MB/s ± 4% 5.54MB/s ± 5% +15.85% (p=0.000 n=20+19) ReferenceFile/struct-32 119MB/s ± 4% 138MB/s ± 5% +15.79% (p=0.000 n=18+20) ReferenceFile/map-32 26.6MB/s ± 7% 27.7MB/s ± 5% +4.06% (p=0.000 n=20+20) HugoFrontMatter-32 11.9MB/s ± 6% 13.9MB/s ± 6% +16.91% (p=0.000 n=19+20) name old alloc/op new alloc/op delta UnmarshalDataset/config-32 16.9MB ± 0% 13.9MB ± 0% -17.48% (p=0.000 n=18+18) UnmarshalDataset/canada-32 74.3MB ± 0% 74.3MB ± 0% -0.00% (p=0.000 n=20+20) UnmarshalDataset/citm_catalog-32 37.3MB ± 0% 37.3MB ± 0% +0.11% (p=0.000 n=20+20) UnmarshalDataset/twitter-32 15.6MB ± 0% 15.6MB ± 0% ~ (p=0.211 n=19+20) UnmarshalDataset/code-32 59.5MB ± 0% 52.4MB ± 0% -11.96% (p=0.000 n=19+20) UnmarshalDataset/example-32 238kB ± 0% 239kB ± 0% +0.02% (p=0.000 n=18+20) UnmarshalSimple/struct-32 981B ± 0% 709B ± 0% -27.73% (p=0.000 n=20+20) UnmarshalSimple/map-32 1.45kB ± 0% 1.17kB ± 0% -18.82% (p=0.000 n=20+20) ReferenceFile/struct-32 11.8kB ± 0% 9.7kB ± 0% -17.64% (p=0.000 n=20+20) ReferenceFile/map-32 51.5kB ± 0% 52.2kB ± 0% +1.30% (p=0.000 n=20+17) HugoFrontMatter-32 12.1kB ± 0% 11.1kB ± 0% -7.97% (p=0.000 n=20+19) name old allocs/op new allocs/op delta UnmarshalDataset/config-32 645k ± 0% 557k ± 0% -13.76% (p=0.000 n=20+16) UnmarshalDataset/canada-32 896k ± 0% 896k ± 0% -0.00% (p=0.000 n=20+20) UnmarshalDataset/citm_catalog-32 380k ± 0% 377k ± 0% -0.75% (p=0.000 n=19+20) UnmarshalDataset/twitter-32 158k ± 0% 158k ± 0% -0.01% (p=0.000 n=18+18) UnmarshalDataset/code-32 2.92M ± 0% 2.57M ± 0% -11.87% (p=0.000 n=20+20) UnmarshalDataset/example-32 3.66k ± 0% 3.64k ± 0% -0.63% (p=0.000 n=20+20) UnmarshalSimple/struct-32 13.0 ± 0% 10.0 ± 0% -23.08% (p=0.000 n=20+20) UnmarshalSimple/map-32 22.0 ± 0% 19.0 ± 0% -13.64% (p=0.000 n=20+20) ReferenceFile/struct-32 253 ± 0% 155 ± 0% -38.74% (p=0.000 n=20+20) ReferenceFile/map-32 1.67k ± 0% 1.44k ± 0% -14.23% (p=0.000 n=20+20) HugoFrontMatter-32 357 ± 0% 313 ± 0% -12.32% (p=0.000 n=20+20) --- benchmark/benchmark_test.go | 122 ++++++++++++++----- internal/tracker/seen.go | 235 ++++++++++++++++++++++-------------- 2 files changed, 235 insertions(+), 122 deletions(-) diff --git a/benchmark/benchmark_test.go b/benchmark/benchmark_test.go index b495140..1337895 100644 --- a/benchmark/benchmark_test.go +++ b/benchmark/benchmark_test.go @@ -10,16 +10,38 @@ import ( ) func BenchmarkUnmarshalSimple(b *testing.B) { - d := struct { - A string - }{} doc := []byte(`A = "hello"`) - for i := 0; i < b.N; i++ { - err := toml.Unmarshal(doc, &d) - if err != nil { - panic(err) + + b.Run("struct", func(b *testing.B) { + b.SetBytes(int64(len(doc))) + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + d := struct { + A string + }{} + + err := toml.Unmarshal(doc, &d) + if err != nil { + panic(err) + } } - } + }) + + b.Run("map", func(b *testing.B) { + b.SetBytes(int64(len(doc))) + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + d := map[string]interface{}{} + err := toml.Unmarshal(doc, &d) + if err != nil { + panic(err) + } + } + }) } type benchmarkDoc struct { @@ -133,33 +155,32 @@ func BenchmarkReferenceFile(b *testing.B) { if err != nil { b.Fatal(err) } - b.SetBytes(int64(len(bytes))) - b.ReportAllocs() - b.ResetTimer() - for i := 0; i < b.N; i++ { - d := benchmarkDoc{} - err := toml.Unmarshal(bytes, &d) - if err != nil { - panic(err) - } - } -} -func BenchmarkReferenceFileMap(b *testing.B) { - bytes, err := ioutil.ReadFile("benchmark.toml") - if err != nil { - b.Fatal(err) - } - b.SetBytes(int64(len(bytes))) - b.ReportAllocs() - b.ResetTimer() - for i := 0; i < b.N; i++ { - m := map[string]interface{}{} - err := toml.Unmarshal(bytes, &m) - if err != nil { - panic(err) + b.Run("struct", func(b *testing.B) { + b.SetBytes(int64(len(bytes))) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + d := benchmarkDoc{} + err := toml.Unmarshal(bytes, &d) + if err != nil { + panic(err) + } } - } + }) + + b.Run("map", func(b *testing.B) { + b.SetBytes(int64(len(bytes))) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + d := map[string]interface{}{} + err := toml.Unmarshal(bytes, &d) + if err != nil { + panic(err) + } + } + }) } func TestReferenceFile(t *testing.T) { @@ -169,3 +190,38 @@ func TestReferenceFile(t *testing.T) { err = toml.Unmarshal(bytes, &d) require.NoError(t, err) } + +func BenchmarkHugoFrontMatter(b *testing.B) { + bytes := []byte(` +categories = ["Development", "VIM"] +date = "2012-04-06" +description = "spf13-vim is a cross platform distribution of vim plugins and resources for Vim." +slug = "spf13-vim-3-0-release-and-new-website" +tags = [".vimrc", "plugins", "spf13-vim", "vim"] +title = "spf13-vim 3.0 release and new website" +include_toc = true +show_comments = false + +[[cascade]] + background = "yosemite.jpg" + [cascade._target] + kind = "page" + lang = "en" + path = "/blog/**" + +[[cascade]] + background = "goldenbridge.jpg" + [cascade._target] + kind = "section" +`) + b.SetBytes(int64(len(bytes))) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + d := map[string]interface{}{} + err := toml.Unmarshal(bytes, &d) + if err != nil { + panic(err) + } + } +} diff --git a/internal/tracker/seen.go b/internal/tracker/seen.go index e245aac..4b5d392 100644 --- a/internal/tracker/seen.go +++ b/internal/tracker/seen.go @@ -1,6 +1,7 @@ package tracker import ( + "bytes" "fmt" "github.com/pelletier/go-toml/v2/internal/ast" @@ -29,67 +30,92 @@ func (k keyKind) String() string { panic("missing keyKind string mapping") } -// SeenTracker tracks which keys have been seen with which TOML type to flag duplicates -// and mismatches according to the spec. +// SeenTracker tracks which keys have been seen with which TOML type to flag +// duplicates and mismatches according to the spec. +// +// Each node in the visited tree is represented by an entry. Each entry has an +// identifier, which is provided by a counter. Entries are stored in the array +// entries. As new nodes are discovered (referenced for the first time in the +// TOML document), entries are created and appended to the array. An entry +// points to its parent using its id. +// +// To find whether a given key (sequence of []byte) has already been visited, +// the entries are linearly searched, looking for one with the right name and +// parent id. +// +// Given that all keys appear in the document after their parent, it is +// guaranteed that all descendants of a node are stored after the node, this +// speeds up the search process. +// +// When encountering [[array tables]], the descendants of that node are removed +// to allow that branch of the tree to be "rediscovered". To maintain the +// invariant above, the deletion process needs to keep the order of entries. +// This results in more copies in that case. type SeenTracker struct { - root *info - current *info + entries []entry + currentIdx int + nextID int } -type info struct { - parent *info +type entry struct { + id int + parent int + name []byte kind keyKind - children map[string]*info explicit bool } -func (i *info) Clear() { - i.children = nil +// Remove all descendent of node at position idx. +func (s *SeenTracker) clear(idx int) { + p := s.entries[idx].id + rest := clear(p, s.entries[idx+1:]) + s.entries = s.entries[:idx+1+len(rest)] } -func (i *info) Has(k string) (*info, bool) { - c, ok := i.children[k] - return c, ok -} - -func (i *info) SetKind(kind keyKind) { - i.kind = kind -} - -func (i *info) CreateTable(k string, explicit bool) *info { - return i.createChild(k, tableKind, explicit) -} - -func (i *info) CreateArrayTable(k string, explicit bool) *info { - return i.createChild(k, arrayTableKind, explicit) -} - -func (i *info) createChild(k string, kind keyKind, explicit bool) *info { - if i.children == nil { - i.children = make(map[string]*info, 1) +func clear(parentID int, entries []entry) []entry { + for i := 0; i < len(entries); { + if entries[i].parent == parentID { + id := entries[i].id + copy(entries[i:], entries[i+1:]) + entries = entries[:len(entries)-1] + rest := clear(id, entries[i:]) + entries = entries[:i+len(rest)] + } else { + i++ + } } + return entries +} - x := &info{ - parent: i, +func (s *SeenTracker) create(parentIdx int, name []byte, kind keyKind, explicit bool) int { + parentID := s.id(parentIdx) + + idx := len(s.entries) + s.entries = append(s.entries, entry{ + id: s.nextID, + parent: parentID, + name: name, kind: kind, explicit: explicit, - } - i.children[k] = x - return x + }) + s.nextID++ + return idx } // CheckExpression takes a top-level node and checks that it does not contain keys // that have been seen in previous calls, and validates that types are consistent. func (s *SeenTracker) CheckExpression(node ast.Node) error { - if s.root == nil { - s.root = &info{ - kind: tableKind, - } - s.current = s.root + if s.entries == nil { + //s.entries = make([]entry, 0, 8) + // Skip ID = 0 to remove the confusion between nodes whose parent has + // id 0 and root nodes (parent id is 0 because it's the zero value). + s.nextID = 1 + // Start unscoped, so idx is negative. + s.currentIdx = -1 } switch node.Kind { case ast.KeyValue: - return s.checkKeyValue(s.current, node) + return s.checkKeyValue(node) case ast.Table: return s.checkTable(node) case ast.ArrayTable: @@ -97,104 +123,135 @@ func (s *SeenTracker) CheckExpression(node ast.Node) error { default: panic(fmt.Errorf("this should not be a top level node type: %s", node.Kind)) } - } -func (s *SeenTracker) checkTable(node ast.Node) error { - s.current = s.root +func (s *SeenTracker) checkTable(node ast.Node) error { it := node.Key() - // handle the first parts of the key, excluding the last one + + parentIdx := -1 + + // This code is duplicated in checkArrayTable. This is because factoring + // it in a function requires to copy the iterator, or allocate it to the + // heap, which is not cheap. for it.Next() { if !it.Node().Next().Valid() { break } - k := string(it.Node().Data) - child, found := s.current.Has(k) - if !found { - child = s.current.CreateTable(k, false) + k := it.Node().Data + + idx := s.find(parentIdx, k) + + if idx < 0 { + idx = s.create(parentIdx, k, tableKind, false) } - s.current = child + parentIdx = idx } - // handle the last part of the key - k := string(it.Node().Data) + k := it.Node().Data + idx := s.find(parentIdx, k) - i, found := s.current.Has(k) - if found { - if i.kind != tableKind { - return fmt.Errorf("toml: key %s should be a table, not a %s", k, i.kind) + if idx >= 0 { + kind := s.entries[idx].kind + if kind != tableKind { + return fmt.Errorf("toml: key %s should be a table, not a %s", string(k), kind) } - if i.explicit { - return fmt.Errorf("toml: table %s already exists", k) + if s.entries[idx].explicit { + return fmt.Errorf("toml: table %s already exists", string(k)) } - i.explicit = true - s.current = i + s.entries[idx].explicit = true } else { - s.current = s.current.CreateTable(k, true) + idx = s.create(parentIdx, k, tableKind, true) } + s.currentIdx = idx + return nil } func (s *SeenTracker) checkArrayTable(node ast.Node) error { - s.current = s.root - it := node.Key() - // handle the first parts of the key, excluding the last one + parentIdx := -1 + for it.Next() { if !it.Node().Next().Valid() { break } - k := string(it.Node().Data) - child, found := s.current.Has(k) - if !found { - child = s.current.CreateTable(k, false) + k := it.Node().Data + + idx := s.find(parentIdx, k) + + if idx < 0 { + idx = s.create(parentIdx, k, tableKind, false) } - s.current = child + parentIdx = idx } - // handle the last part of the key - k := string(it.Node().Data) + k := it.Node().Data + idx := s.find(parentIdx, k) - info, found := s.current.Has(k) - if found { - if info.kind != arrayTableKind { - return fmt.Errorf("toml: key %s already exists as a %s, but should be an array table", info.kind, k) + if idx >= 0 { + kind := s.entries[idx].kind + if kind != arrayTableKind { + return fmt.Errorf("toml: key %s already exists as a %s, but should be an array table", kind, string(k)) } - info.Clear() + s.clear(idx) } else { - info = s.current.CreateArrayTable(k, true) + idx = s.create(parentIdx, k, arrayTableKind, true) } - s.current = info + s.currentIdx = idx + return nil } -func (s *SeenTracker) checkKeyValue(context *info, node ast.Node) error { +func (s *SeenTracker) checkKeyValue(node ast.Node) error { it := node.Key() - // handle the first parts of the key, excluding the last one + parentIdx := s.currentIdx + for it.Next() { - k := string(it.Node().Data) - child, found := context.Has(k) - if found { - if child.kind != tableKind { - return fmt.Errorf("toml: expected %s to be a table, not a %s", k, child.kind) + k := it.Node().Data + + idx := s.find(parentIdx, k) + + if idx >= 0 { + if s.entries[idx].kind != tableKind { + return fmt.Errorf("toml: expected %s to be a table, not a %s", string(k), s.entries[idx].kind) } } else { - child = context.CreateTable(k, false) + idx = s.create(parentIdx, k, tableKind, false) } - context = child + parentIdx = idx } + kind := valueKind + if node.Value().Kind == ast.InlineTable { - context.SetKind(tableKind) - } else { - context.SetKind(valueKind) + kind = tableKind } + s.entries[parentIdx].kind = kind return nil } + +func (s *SeenTracker) id(idx int) int { + if idx >= 0 { + return s.entries[idx].id + } + return 0 +} + +func (s *SeenTracker) find(parentIdx int, k []byte) int { + parentID := s.id(parentIdx) + + for i := parentIdx + 1; i < len(s.entries); i++ { + if s.entries[i].parent == parentID && bytes.Equal(s.entries[i].name, k) { + return i + } + } + + return -1 +}