From 803e6ba61e5b3de9effcda4589a51216cc547218 Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Wed, 11 Jan 2023 11:59:28 +0900 Subject: [PATCH] Separate interpreter and engine per wazero.cache impl (#1022) Signed-off-by: Takeshi Yoneda --- .github/workflows/commit.yaml | 2 +- Makefile | 10 +++++----- cache.go | 18 ++++++++++------- cache_test.go | 37 ++++++++++++++++++++++++++++++++--- config.go | 13 ++++++++++-- config_test.go | 14 +++++++++++++ runtime.go | 4 +--- runtime_test.go | 13 ++++++++++-- 8 files changed, 88 insertions(+), 23 deletions(-) diff --git a/.github/workflows/commit.yaml b/.github/workflows/commit.yaml index e5027722..5f2a759e 100644 --- a/.github/workflows/commit.yaml +++ b/.github/workflows/commit.yaml @@ -79,7 +79,7 @@ jobs: if: ${{ github.event_name == 'pull_request' }} # Run tests with -race only on main branch push. - - run: make test go_test_options='-race' + - run: make test go_test_options='-timeout 10m -race' if: ${{ github.event_name == 'push' }} - name: "Generate coverage report" # only once (not per OS) diff --git a/Makefile b/Makefile index 46dbd96c..642b6502 100644 --- a/Makefile +++ b/Makefile @@ -23,7 +23,7 @@ main_sources := $(wildcard $(filter-out %_test.go $(all_testdata) $(all_testing main_packages := $(sort $(foreach f,$(dir $(main_sources)),$(if $(findstring ./,$(f)),./,./$(f)))) # By default, we don't run with -race as it's costly to run on every PR. -go_test_options ?= +go_test_options ?= -timeout 120s ensureCompilerFastest := -ldflags '-X github.com/tetratelabs/wazero/internal/integration_test/vs.ensureCompilerFastest=true' .PHONY: bench @@ -177,8 +177,8 @@ build.spectest.v2: # Note: SIMD cases are placed in the "simd" subdirectory. .PHONY: test test: - @go test $(go_test_options) $$(go list ./... | grep -vE '$(spectest_v1_dir)|$(spectest_v2_dir)') -timeout 120s - @cd internal/version/testdata && go test $(go_test_options) ./... -timeout 120s + @go test $(go_test_options) $$(go list ./... | grep -vE '$(spectest_v1_dir)|$(spectest_v2_dir)') + @cd internal/version/testdata && go test $(go_test_options) ./... .PHONY: coverage coverpkg = $(subst $(space),$(comma),$(main_packages)) @@ -192,10 +192,10 @@ spectest: @$(MAKE) spectest.v2 spectest.v1: - @go test $(go_test_options) $$(go list ./... | grep $(spectest_v1_dir)) -timeout 120s + @go test $(go_test_options) $$(go list ./... | grep $(spectest_v1_dir)) spectest.v2: - @go test $(go_test_options) $$(go list ./... | grep $(spectest_v2_dir)) -timeout 120s + @go test $(go_test_options) $$(go list ./... | grep $(spectest_v2_dir)) golangci_lint_path := $(shell go env GOPATH)/bin/golangci-lint diff --git a/cache.go b/cache.go index 34bb53d6..acddb862 100644 --- a/cache.go +++ b/cache.go @@ -51,20 +51,24 @@ func NewCompilationCacheWithDir(dirname string) (CompilationCache, error) { type cache struct { // eng is the engine for this cache. If the cache is configured, the engine is shared across multiple instances of // Runtime, and its lifetime is not bound to them. Instead, the engine is alive until Cache.Close is called. - eng wasm.Engine + engs [engineKindCount]wasm.Engine fileCache filecache.Cache - once sync.Once + initOnces [engineKindCount]sync.Once } -func (c *cache) initEngine(ne newEngine, ctx context.Context, features api.CoreFeatures) wasm.Engine { - c.once.Do(func() { c.eng = ne(ctx, features, c.fileCache) }) - return c.eng +func (c *cache) initEngine(ek engineKind, ne newEngine, ctx context.Context, features api.CoreFeatures) wasm.Engine { + c.initOnces[ek].Do(func() { c.engs[ek] = ne(ctx, features, c.fileCache) }) + return c.engs[ek] } // Close implements the same method on the Cache interface. func (c *cache) Close(_ context.Context) (err error) { - if c.eng != nil { - err = c.eng.Close() + for _, eng := range c.engs { + if eng != nil { + if err = eng.Close(); err != nil { + return + } + } } return } diff --git a/cache_test.go b/cache_test.go index 665a8c88..1fd145cd 100644 --- a/cache_test.go +++ b/cache_test.go @@ -9,7 +9,9 @@ import ( goruntime "runtime" "testing" + "github.com/tetratelabs/wazero/internal/platform" "github.com/tetratelabs/wazero/internal/testing/require" + "github.com/tetratelabs/wazero/internal/wasm" ) //go:embed internal/integration_test/vs/testdata/fac.wasm @@ -22,11 +24,17 @@ func TestCompilationCache(t *testing.T) { foo, bar := getCacheSharedRuntimes(ctx, t) cacheInst := foo.cache + // add interpreter first, to ensure compiler support isn't order dependent + eng := foo.cache.engs[engineKindInterpreter] + if platform.CompilerSupported() { + eng = foo.cache.engs[engineKindCompiler] + } + // Try compiling. compiled, err := foo.CompileModule(ctx, facWasm) require.NoError(t, err) // Also check it is actually cached. - require.Equal(t, uint32(1), cacheInst.eng.CompiledModuleCount()) + require.Equal(t, uint32(1), eng.CompiledModuleCount()) barCompiled, err := bar.CompileModule(ctx, facWasm) require.NoError(t, err) @@ -47,12 +55,12 @@ func TestCompilationCache(t *testing.T) { require.NoError(t, err) err = bar.Close(ctx) require.NoError(t, err) - require.Equal(t, uint32(1), cacheInst.eng.CompiledModuleCount()) + require.Equal(t, uint32(1), eng.CompiledModuleCount()) // Close the cache, and ensure the engine is closed. err = cacheInst.Close(ctx) require.NoError(t, err) - require.Equal(t, uint32(0), cacheInst.eng.CompiledModuleCount()) + require.Equal(t, uint32(0), eng.CompiledModuleCount()) }) // Even when cache is configured, compiled host modules must be different as that's the way @@ -162,3 +170,26 @@ func requireChdirToTemp(t *testing.T) (string, string) { require.NoError(t, os.Chdir(tmpDir)) return tmpDir, oldwd } + +func TestCache_Close(t *testing.T) { + t.Run("all engines", func(t *testing.T) { + c := &cache{engs: [engineKindCount]wasm.Engine{&mockEngine{}, &mockEngine{}}} + err := c.Close(testCtx) + require.NoError(t, err) + for i := engineKind(0); i < engineKindCount; i++ { + require.True(t, c.engs[i].(*mockEngine).closed) + } + }) + t.Run("only interp", func(t *testing.T) { + c := &cache{engs: [engineKindCount]wasm.Engine{nil, &mockEngine{}}} + err := c.Close(testCtx) + require.NoError(t, err) + require.True(t, c.engs[engineKindInterpreter].(*mockEngine).closed) + }) + t.Run("only compiler", func(t *testing.T) { + c := &cache{engs: [engineKindCount]wasm.Engine{&mockEngine{}, nil}} + err := c.Close(testCtx) + require.NoError(t, err) + require.True(t, c.engs[engineKindCompiler].(*mockEngine).closed) + }) +} diff --git a/config.go b/config.go index a8c4bc4a..65ae437a 100644 --- a/config.go +++ b/config.go @@ -131,7 +131,7 @@ type runtimeConfig struct { enabledFeatures api.CoreFeatures memoryLimitPages uint32 memoryCapacityFromMax bool - isInterpreter bool + engineKind engineKind dwarfDisabled bool // negative as defaults to enabled newEngine newEngine cache CompilationCache @@ -145,6 +145,14 @@ var engineLessConfig = &runtimeConfig{ dwarfDisabled: false, } +type engineKind int + +const ( + engineKindCompiler engineKind = iota + engineKindInterpreter + engineKindCount +) + // NewRuntimeConfigCompiler compiles WebAssembly modules into // runtime.GOARCH-specific assembly for optimal performance. // @@ -161,6 +169,7 @@ var engineLessConfig = &runtimeConfig{ // NewRuntimeConfigInterpreter if needed. func NewRuntimeConfigCompiler() RuntimeConfig { ret := engineLessConfig.clone() + ret.engineKind = engineKindCompiler ret.newEngine = compiler.NewEngine return ret } @@ -168,7 +177,7 @@ func NewRuntimeConfigCompiler() RuntimeConfig { // NewRuntimeConfigInterpreter interprets WebAssembly modules instead of compiling them into assembly. func NewRuntimeConfigInterpreter() RuntimeConfig { ret := engineLessConfig.clone() - ret.isInterpreter = true + ret.engineKind = engineKindInterpreter ret.newEngine = interpreter.NewEngine return ret } diff --git a/config_test.go b/config_test.go index 65138278..c41e6654 100644 --- a/config_test.go +++ b/config_test.go @@ -10,6 +10,7 @@ import ( "github.com/tetratelabs/wazero/api" "github.com/tetratelabs/wazero/internal/fstest" + "github.com/tetratelabs/wazero/internal/platform" internalsys "github.com/tetratelabs/wazero/internal/sys" testfs "github.com/tetratelabs/wazero/internal/testing/fs" "github.com/tetratelabs/wazero/internal/testing/require" @@ -620,6 +621,19 @@ func Test_compiledModule_Close(t *testing.T) { } } +func TestNewRuntimeConfig(t *testing.T) { + c, ok := NewRuntimeConfig().(*runtimeConfig) + require.True(t, ok) + // Should be cloned from the source. + require.NotEqual(t, engineLessConfig, c) + // Ensures if the correct engine is selected. + if platform.CompilerSupported() { + require.Equal(t, engineKindCompiler, c.engineKind) + } else { + require.Equal(t, engineKindInterpreter, c.engineKind) + } +} + // requireSysContext ensures wasm.NewContext doesn't return an error, which makes it usable in test matrices. func requireSysContext( t *testing.T, diff --git a/runtime.go b/runtime.go index 2634886c..1a6eafb1 100644 --- a/runtime.go +++ b/runtime.go @@ -116,7 +116,7 @@ func NewRuntimeWithConfig(ctx context.Context, rConfig RuntimeConfig) Runtime { if c := config.cache; c != nil { // If the Cache is configured, we share the engine. cacheImpl = c.(*cache) - engine = cacheImpl.initEngine(config.newEngine, ctx, config.enabledFeatures) + engine = cacheImpl.initEngine(config.engineKind, config.newEngine, ctx, config.enabledFeatures) } else { // Otherwise, we create a new engine. engine = config.newEngine(ctx, config.enabledFeatures, nil) @@ -128,7 +128,6 @@ func NewRuntimeWithConfig(ctx context.Context, rConfig RuntimeConfig) Runtime { enabledFeatures: config.enabledFeatures, memoryLimitPages: config.memoryLimitPages, memoryCapacityFromMax: config.memoryCapacityFromMax, - isInterpreter: config.isInterpreter, dwarfDisabled: config.dwarfDisabled, } } @@ -140,7 +139,6 @@ type runtime struct { enabledFeatures api.CoreFeatures memoryLimitPages uint32 memoryCapacityFromMax bool - isInterpreter bool dwarfDisabled bool } diff --git a/runtime_test.go b/runtime_test.go index fbd91a2f..5836d2f6 100644 --- a/runtime_test.go +++ b/runtime_test.go @@ -12,6 +12,7 @@ import ( "github.com/tetratelabs/wazero/experimental" "github.com/tetratelabs/wazero/internal/filecache" "github.com/tetratelabs/wazero/internal/leb128" + "github.com/tetratelabs/wazero/internal/platform" "github.com/tetratelabs/wazero/internal/testing/require" "github.com/tetratelabs/wazero/internal/version" "github.com/tetratelabs/wazero/internal/wasm" @@ -702,12 +703,20 @@ func (e *mockEngine) Close() (err error) { func TestNewRuntime_concurrent(t *testing.T) { const num = 100 var wg sync.WaitGroup - config := NewRuntimeConfig().WithCompilationCache(NewCompilationCache()) + c := NewCompilationCache() + // If available, uses two engine configurations for the single compilation cache. + configs := [2]RuntimeConfig{NewRuntimeConfigInterpreter().WithCompilationCache(c)} + if platform.CompilerSupported() { + configs[1] = NewRuntimeConfigCompiler().WithCompilationCache(c) + } else { + configs[1] = NewRuntimeConfigInterpreter().WithCompilationCache(c) + } wg.Add(num) for i := 0; i < num; i++ { + i := i go func() { defer wg.Done() - r := NewRuntimeWithConfig(testCtx, config) + r := NewRuntimeWithConfig(testCtx, configs[i%2]) err := r.Close(testCtx) require.NoError(t, err) }()