Repository: tetratelabs/wazero
Pull request #117 (2022-01-06 05:24:16)

Text Format: reduces the amount of parsing state
This generifies some parser state so that it can be reused next for
parsing parameter names. This also cleans up some ++ stuff.

codefromthecrypt: thanks for the look @mathetake and PS I measured and it is same perf and allocation count. If we tried to track names with string pointers, that increases allocations, so taking advantage of the fact we already have []byte here, we end up a wash even with the less code.

Patch:
diff --git a/wasm/wat/lexer.go b/wasm/wat/lexer.go
index 65f70189..c650c8b2 100644
--- a/wasm/wat/lexer.go
+++ b/wasm/wat/lexer.go
@@ -54,7 +54,7 @@ func lex(parser tokenParser, source []byte) (line, col uint32, err error) {
 		// The spec does not consider newlines apart from '\n'. Notably, a bare '\r' is not a newline here.
 		// See https://www.w3.org/TR/wasm-core-1/#text-comment
 		if b1 == '\n' {
-			line = line + 1
+			line++
 			col = 0  // for loop will + 1
 			continue // next line
 		}
@@ -72,14 +72,14 @@ func lex(parser tokenParser, source []byte) (line, col uint32, err error) {
 			}
 			if source[peek] == ';' { // next block comment
 				i = peek // continue after "(;"
-				col = col + 1
-				blockCommentDepth = blockCommentDepth + 1
+				col++
+				blockCommentDepth++
 				continue
 			} else if blockCommentDepth == 0 { // Fast path left paren token at the expense of code duplication.
 				if e := parser(tokenLParen, constantLParen, line, col); e != nil {
 					return line, col, e
 				}
-				parenDepth = parenDepth + 1
+				parenDepth++
 				continue
 			}
 		case ')':
@@ -90,7 +90,7 @@ func lex(parser tokenParser, source []byte) (line, col uint32, err error) {
 				if e := parser(tokenRParen, constantRParen, line, col); e != nil {
 					return line, col, e
 				}
-				parenDepth = parenDepth - 1
+				parenDepth--
 				continue
 			}
 		case ';': // possible line comment or block comment end
@@ -99,15 +99,15 @@ func lex(parser tokenParser, source []byte) (line, col uint32, err error) {
 				b2 := source[peek]
 				if blockCommentDepth > 0 && b2 == ')' {
 					i = peek // continue after ";)"
-					col = col + 1
-					blockCommentDepth = blockCommentDepth - 1
+					col++
+					blockCommentDepth--
 					continue
 				}
 
 				if b2 == ';' { // line comment
 					// Start after ";;" and run until the end. Note UTF-8 (multi-byte) characters are allowed.
-					peek = peek + 1
-					col = col + 1
+					peek++
+					col++
 
 				LineComment:
 					for peek < end {
@@ -116,7 +116,7 @@ func lex(parser tokenParser, source []byte) (line, col uint32, err error) {
 							break LineComment // EOL bookkeeping will proceed on the next iteration
 						}
 
-						col = col + 1
+						col++
 						s := utf8Size[peeked] // While unlikely, it is possible the byte peeked is invalid unicode
 						if s == 0 {
 							return line, col, fmt.Errorf("found an invalid byte in line comment: 0x%x", peeked)
@@ -167,11 +167,11 @@ func lex(parser tokenParser, source []byte) (line, col uint32, err error) {
 				}
 			Number:
 				// Start after the number and run until the end. Note all allowed characters are single byte.
-				for ; peek < end; peek = peek + 1 {
+				for ; peek < end; peek++ {
 					switch source[peek] {
 					case '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '_':
 						i = peek
-						col = col + 1
+						col++
 					default:
 						break Number // end of this token (or malformed, which the next loop will notice)
 					}
@@ -188,7 +188,7 @@ func lex(parser tokenParser, source []byte) (line, col uint32, err error) {
 					break String
 				}
 
-				col = col + 1
+				col++
 				s := utf8Size[peeked] // While unlikely, it is possible the current byte is invalid unicode
 				if s == 0 {
 					return line, col, fmt.Errorf("found an invalid byte in string token: 0x%x", peeked)
@@ -202,16 +202,16 @@ func lex(parser tokenParser, source []byte) (line, col uint32, err error) {
 
 			i = peek
 			// set the position to after the quote
-			peek = peek + 1
-			col = col + 1
+			peek++
+			col++
 		case tokenKeyword, tokenID, tokenReserved: // min 1 byte; end with zero or more idChar
 			// Start after the first character and run until the end. Note all allowed characters are single byte.
 		IdChars:
-			for ; peek < end; peek = peek + 1 {
+			for ; peek < end; peek++ {
 				if !idChar[source[peek]] {
 					break IdChars // end of this token (or malformed, which the next loop will notice)
 				}
-				col = col + 1
+				col++
 			}
 			i = peek - 1
 		default:
diff --git a/wasm/wat/parser.go b/wasm/wat/parser.go
index c000dc31..42d25500 100644
--- a/wasm/wat/parser.go
+++ b/wasm/wat/parser.go
@@ -5,37 +5,51 @@ import (
 	"fmt"
 )
 
+// currentField holds the positional state of parser. Values are also useful as they allow you to do a reference search
+// for all related code including parsers of that position.
 type currentField byte
 
 const (
 	// fieldInitial is the first position in the source being parsed.
 	fieldInitial currentField = iota
+	// fieldModule is a top-level field named "module"
 	fieldModule
+	// fieldModuleImport is a field named "import" enclosed by the top-level field "module"
+	//
+	// At the start of the field, moduleParser.currentValue0 tracks importFunc.module while moduleParser.currentValue1
+	// tracks importFunc.name. If a field named "func" is encountered, these names are recorded while
+	// fieldModuleImportFunc takes over parsing.
 	fieldModuleImport
+	// fieldModuleImportFunc is a field named "func" enclosed by a field named "import"
 	fieldModuleImportFunc
+	// fieldModuleStart is a field named "start" enclosed by the top-level field "module"
 	fieldModuleStart
 )
 
 type moduleParser struct {
+	// source is the entire WebAssembly text format source code being parsed.
 	source []byte
+	// module holds the fields incrementally parsed from tokens in the source.
 	module *module
-	// currentStringCount allows us to unquote the _import.module and _import.name fields, differentiating empty
-	// from never set, without making _import.module and _import.name pointers.
-	currentStringCount int
+
 	// currentField is the parser and error context.
 	// This is set after reading a field name, ex "module", or after reaching the end of one, ex ')'.
 	currentField
+
 	// tokenParser is called by lex, and changes based on the currentField.
 	// The initial tokenParser is ensureLParen because %.wat files must begin with a '(' token (ignoring whitespace).
 	tokenParser
+
+	// currentValue0 is a currentField-specific place to stash a string when parsing a field.
+	// Ex. for fieldModuleImport, this would be Math if (import "Math" "PI" ...)
+	currentValue0 []byte
+
+	// currentValue1 is a currentField-specific place to stash a string when parsing a field.
+	// Ex. for fieldModuleImport, this would be PI if (import "Math" "PI" ...)
+	currentValue1 []byte
+
 	// currentImportIndex allows us to track the relative position of imports regardless of what they describe
-	currentImportIndex int
-	// currentImportModule is the first string of the current import. Ex. "Math" if (import "Math" "PI" (func))
-	currentImportModule string
-	// currentImportName is the second string of the current import. Ex. "PI" if (import "Math" "PI" (func))
-	currentImportName string
-	// currentImportHasDesc tracks if we found a description (Ex. (func)) of the current import.
-	currentImportHasDesc bool
+	currentImportIndex uint32
 }
 
 // parseModule parses the configured source into a module. This function returns when the source is exhausted or an
@@ -140,34 +154,27 @@ func (p *moduleParser) parseImport(tok tokenType, tokenBytes []byte, _, _ uint32
 	switch tok {
 	case tokenString:
 		// Note: tokenString is minimum length two on account of quotes. Ex. "" or "foo"
-		name := string(tokenBytes[1 : len(tokenBytes)-1]) // unquote
-		if p.currentStringCount == 0 {
-			p.currentImportModule = name
-		} else if p.currentImportName != "" {
+		name := tokenBytes[1 : len(tokenBytes)-1] // unquote
+		if p.currentValue0 == nil {               // Ex. (module ""
+			p.currentValue0 = name
+		} else if p.currentValue1 == nil { // Ex. (module "" ""
+			p.currentValue1 = name
+		} else { // Ex. (module "" "" ""
 			return fmt.Errorf("redundant name: %s", name)
-		} else {
-			p.currentImportName = name
 		}
-		p.currentStringCount = p.currentStringCount + 1
 	case tokenLParen: // start fields, ex. (func
 		p.tokenParser = p.startField
 		return nil
 	case tokenRParen: // end of this import
-		switch p.currentStringCount { // names precede desc
-		case 0:
-			return errors.New("missing module and name")
-		case 1:
-			return errors.New("missing name")
-		}
-		if !p.currentImportHasDesc {
-			return errors.New("missing description")
+		// If we've not yet added the current import, determine the best error message.
+		if uint32(len(p.module.importFuncs)) == p.currentImportIndex {
+			if err := p.validateImportModuleAndName(); err != nil {
+				return err // Ex. (module) or (module "Math")
+			}
+			return errors.New("missing description field") // Ex. (module "Math" "Pi")
 		}
 		p.currentField = fieldModule
-		p.currentImportIndex = p.currentImportIndex + 1
-		p.currentStringCount = 0
-		p.currentImportModule = ""
-		p.currentImportName = ""
-		p.currentImportHasDesc = false
+		p.currentImportIndex++
 		p.tokenParser = p.parseModule
 	default:
 		return p.unexpectedToken(tok, tokenBytes)
@@ -176,20 +183,36 @@ func (p *moduleParser) parseImport(tok tokenType, tokenBytes []byte, _, _ uint32
 }
 
 func (p *moduleParser) importFieldHandler(fieldName []byte) (tokenParser, error) {
-	if p.currentImportHasDesc {
+	if uint32(len(p.module.importFuncs)) > p.currentImportIndex {
 		return nil, fmt.Errorf("redundant field: %s", string(fieldName))
 	}
 	switch string(fieldName) {
 	case "func":
-		p.currentImportHasDesc = true
+		if err := p.validateImportModuleAndName(); err != nil {
+			return nil, err
+		}
 		p.currentField = fieldModuleImportFunc
-		desc := &importFunc{module: p.currentImportModule, name: p.currentImportName, importIndex: p.currentImportIndex}
+		desc := &importFunc{
+			module:      string(p.currentValue0),
+			name:        string(p.currentValue1),
+			importIndex: p.currentImportIndex,
+		}
+		p.currentValue0, p.currentValue1 = nil, nil
 		p.module.importFuncs = append(p.module.importFuncs, desc)
 		return p.parseImportFunc, nil
 	} // TODO: table, memory or global
 	return nil, fmt.Errorf("unexpected field: %s", string(fieldName))
 }
 
+func (p *moduleParser) validateImportModuleAndName() error {
+	if p.currentValue0 == nil && p.currentValue1 == nil {
+		return errors.New("missing module and name")
+	} else if p.currentValue1 == nil {
+		return errors.New("missing name")
+	}
+	return nil
+}
+
 func (p *moduleParser) parseImportFunc(tok tokenType, tokenBytes []byte, _, _ uint32) error {
 	switch tok {
 	case tokenID: // Ex. $main
diff --git a/wasm/wat/types.go b/wasm/wat/types.go
index e0b5c4aa..c61e5d45 100644
--- a/wasm/wat/types.go
+++ b/wasm/wat/types.go
@@ -82,10 +82,10 @@ var typeFuncEmpty = &typeFunc{}
 // See https://www.w3.org/TR/wasm-core-1/#imports%E2%91%A0
 type importFunc struct {
 	// importIndex is the zero-based index in module.imports. This is needed because imports are not always functions.
-	importIndex int
+	importIndex uint32
 
 	// typeIndex is the zero-based index in module.types representing this function signature.
-	typeIndex int
+	typeIndex uint32
 
 	// module is the possibly empty module name to import. Ex. "" or "Math"
 	//


Test patch:
diff --git a/wasm/wat/names_test.go b/wasm/wat/names_test.go
index f4419ab9..fbc13756 100644
--- a/wasm/wat/names_test.go
+++ b/wasm/wat/names_test.go
@@ -15,15 +15,15 @@ func TestEncodeNameSection(t *testing.T) {
 	// TIP: the below is the binary suffix of `wat2wasm --debug-names --debug-parser -v simple.wat` where simple.wat
 	// contains the same text as simpleExample
 	require.Equal(t, []byte{
-		0x00, /* module subsection ID zero */
-		0x07, /* 7 bytes to follow */
-		0x06, /* the module name simple is 6 characters long */
+		0x00, // module subsection ID zero
+		0x07, // 7 bytes to follow
+		0x06, // the module name simple is 6 characters long
 		's', 'i', 'm', 'p', 'l', 'e',
-		0x01, /* function subsection ID one */
-		0x08, /* 8 bytes to follow */
-		0x01, /* one function name */
-		0x00, /* the function index is zero */
-		0x05, /* the function name hello is 5 characters long */
+		0x01, // function subsection ID one
+		0x08, // 8 bytes to follow
+		0x01, // one function name
+		0x00, // the function index is zero
+		0x05, // the function name hello is 5 characters long
 		'h', 'e', 'l', 'l', 'o',
 	}, encodeNameSection(m))
 }
@@ -46,7 +46,7 @@ func TestEncodeNameSection_OnlyFuncName(t *testing.T) {
 
 	expected := append(append([]byte{
 		0x01, // function subsection ID one
-		// length includes 1 byte overhead for the function name count, and 2 bytes (index + length prefix) per name
+		// length includes overhead for size in bytes of the function name count, plus index + length prefix per name
 		byte(1 + 2 + 2 + len(func0) + len(func1)),
 		0x02, // two function names
 	},
diff --git a/wasm/wat/parser_test.go b/wasm/wat/parser_test.go
index 5cc9f525..a215087b 100644
--- a/wasm/wat/parser_test.go
+++ b/wasm/wat/parser_test.go
@@ -133,7 +133,7 @@ func TestParseModule_Errors(t *testing.T) {
 		{
 			name:        "import missing desc",
 			input:       "(module (import \"foo\" \"bar\"))",
-			expectedErr: "1:28: missing description in module.import[0]",
+			expectedErr: "1:28: missing description field in module.import[0]",
 		},
 		{
 			name:        "import empty desc",