From 28dd2f6e19ccff78d8660c33edc62a689e805c05 Mon Sep 17 00:00:00 2001 From: Hajime Hoshi Date: Mon, 12 Aug 2019 20:09:17 +0900 Subject: [PATCH] shareable: Refactoring: only finalizers have to be cared Only finalizers are problematic since they can stop everything, but other things are not. Let's care finalizers and just use regular locks. Updates #913 --- internal/restorable/image.go | 69 +++++++-------------------------- internal/restorable/images.go | 31 ++------------- internal/shareable/shareable.go | 27 ++++++++----- 3 files changed, 35 insertions(+), 92 deletions(-) diff --git a/internal/restorable/image.go b/internal/restorable/image.go index cc50b0d85..aaf29bda7 100644 --- a/internal/restorable/image.go +++ b/internal/restorable/image.go @@ -121,7 +121,8 @@ func init() { // // Note that Dispose is not called automatically. func NewImage(width, height int) *Image { - // As this should not affect the information for restoring, this doesn't have to be deferred. + theImages.m.Lock() + defer theImages.m.Unlock() i := &Image{ image: graphicscommand.NewImage(width, height), @@ -174,7 +175,8 @@ func (i *Image) MakeVolatile() { // // Note that Dispose is not called automatically. func NewScreenFramebufferImage(width, height int) *Image { - // As this should not affect the information for restoring, this doesn't have to be deferred. + theImages.m.Lock() + defer theImages.m.Unlock() i := &Image{ image: graphicscommand.NewScreenFramebufferImage(width, height), @@ -186,18 +188,8 @@ func NewScreenFramebufferImage(width, height int) *Image { } func (i *Image) Clear() { - select { - case theImages.deferCh <- struct{}{}: - break - default: - theImages.deferUntilBeginFrame(i.Clear) - return - } - - defer func() { - <-theImages.deferCh - }() - + theImages.m.Lock() + defer theImages.m.Unlock() theImages.makeStaleIfDependingOn(i) i.clear() } @@ -308,19 +300,8 @@ func (i *Image) ClearPixels(x, y, width, height int) { // // ReplacePixels for a part is forbidden if the image is rendered with DrawTriangles or Fill. func (i *Image) ReplacePixels(pixels []byte, x, y, width, height int) { - select { - case theImages.deferCh <- struct{}{}: - break - default: - theImages.deferUntilBeginFrame(func() { - i.ReplacePixels(pixels, x, y, width, height) - }) - return - } - - defer func() { - <-theImages.deferCh - }() + theImages.m.Lock() + defer theImages.m.Unlock() w, h := i.image.Size() if width <= 0 || height <= 0 { @@ -375,19 +356,8 @@ func (i *Image) ReplacePixels(pixels []byte, x, y, width, height int) { // DrawTriangles draws a given image img to the image. func (i *Image) DrawTriangles(img *Image, vertices []float32, indices []uint16, colorm *affine.ColorM, mode driver.CompositeMode, filter driver.Filter, address driver.Address) { - select { - case theImages.deferCh <- struct{}{}: - break - default: - theImages.deferUntilBeginFrame(func() { - i.DrawTriangles(img, vertices, indices, colorm, mode, filter, address) - }) - return - } - - defer func() { - <-theImages.deferCh - }() + theImages.m.Lock() + defer theImages.m.Unlock() if i.priority { panic("restorable: DrawTriangles cannot be called on a priority image") @@ -443,11 +413,8 @@ func (i *Image) readPixelsFromGPUIfNeeded() { // // Note that this must not be called until context is available. func (i *Image) At(x, y int) (byte, byte, byte, byte) { - // As At can be affected by deferred functions, and At itself cannot be deferred, lock this operation. - theImages.deferCh <- struct{}{} - defer func() { - <-theImages.deferCh - }() + theImages.m.Lock() + defer theImages.m.Unlock() w, h := i.image.Size() if x < 0 || y < 0 || w <= x || h <= y { @@ -574,16 +541,8 @@ func (i *Image) restore() { // // After disposing, calling the function of the image causes unexpected results. func (i *Image) Dispose() { - select { - case theImages.deferCh <- struct{}{}: - break - default: - theImages.deferUntilBeginFrame(i.Dispose) - return - } - defer func() { - <-theImages.deferCh - }() + theImages.m.Lock() + defer theImages.m.Unlock() theImages.remove(i) i.image.Dispose() diff --git a/internal/restorable/images.go b/internal/restorable/images.go index c26eb7288..44a6549fa 100644 --- a/internal/restorable/images.go +++ b/internal/restorable/images.go @@ -42,17 +42,13 @@ type images struct { images map[*Image]struct{} lastTarget *Image - deferred []func() - mDeferred sync.Mutex - deferCh chan struct{} - + m sync.Mutex once sync.Once } // theImages represents the images for the current process. var theImages = &images{ - images: map[*Image]struct{}{}, - deferCh: make(chan struct{}, 1), + images: map[*Image]struct{}{}, } // ResolveStaleImages flushes the queued draw commands and resolves @@ -62,7 +58,7 @@ var theImages = &images{ func ResolveStaleImages() { defer func() { // Until the begin o the frame (by RestoreIfNeeded, any operations are deferred. - theImages.deferCh <- struct{}{} + theImages.m.Lock() }() graphicscommand.FlushCommands() @@ -89,12 +85,9 @@ func RestoreIfNeeded() error { firsttime = true }) if !firsttime { - <-theImages.deferCh + theImages.m.Unlock() } - // Deferred functions should be resolved after restoring. - defer theImages.resolveDeferred() - if !needsRestoring() { return nil } @@ -146,22 +139,6 @@ func (i *images) remove(img *Image) { delete(i.images, img) } -func (i *images) deferUntilBeginFrame(f func()) { - i.mDeferred.Lock() - defer i.mDeferred.Unlock() - i.deferred = append(i.deferred, f) -} - -func (i *images) resolveDeferred() { - i.mDeferred.Lock() - defer i.mDeferred.Unlock() - - for _, f := range i.deferred { - f() - } - i.deferred = nil -} - // resolveStaleImages resolves stale images. func (i *images) resolveStaleImages() { i.lastTarget = nil diff --git a/internal/shareable/shareable.go b/internal/shareable/shareable.go index 2d92c8479..ef6427b40 100644 --- a/internal/shareable/shareable.go +++ b/internal/shareable/shareable.go @@ -382,20 +382,27 @@ func (i *Image) at(x, y int) (byte, byte, byte, byte) { return i.backend.restorable.At(x+ox, y+oy) } -func (i *Image) Dispose() { +// disposeFromFinalizer disposes images, but the actual operation is deferred. +// disposeFromFinalizer is called from finalizers. +// +// A function from finalizer must not be blocked, but disposing operation can be blocked. +// Defer this operation until it becomes safe. (#913) +func (i *Image) disposeFromFinalizer() { deferredM.Lock() defer deferredM.Unlock() - // Defer actual disposing until disposing can finish completely. - // Otherwise, ClearPixels can be deferred in between frames, and delayed ClearPixels can affect other images. - // (#913) - // - // TODO: Other operations should be deferred too? deferred = append(deferred, func() { i.dispose(true) }) } +func (i *Image) Dispose() { + backendsM.Lock() + defer backendsM.Unlock() + + i.dispose(true) +} + func (i *Image) dispose(markDisposed bool) { defer func() { if markDisposed { @@ -480,7 +487,7 @@ func (i *Image) allocate(shareable bool) { i.backend = &backend{ restorable: restorable.NewImage(i.width, i.height), } - runtime.SetFinalizer(i, (*Image).Dispose) + runtime.SetFinalizer(i, (*Image).disposeFromFinalizer) return } @@ -488,7 +495,7 @@ func (i *Image) allocate(shareable bool) { if n, ok := b.TryAlloc(i.width, i.height); ok { i.backend = b i.node = n - runtime.SetFinalizer(i, (*Image).Dispose) + runtime.SetFinalizer(i, (*Image).disposeFromFinalizer) return } } @@ -512,7 +519,7 @@ func (i *Image) allocate(shareable bool) { } i.backend = b i.node = n - runtime.SetFinalizer(i, (*Image).Dispose) + runtime.SetFinalizer(i, (*Image).disposeFromFinalizer) } func (i *Image) MakeVolatile() { @@ -544,7 +551,7 @@ func NewScreenFramebufferImage(width, height int) *Image { }, neverShared: true, } - runtime.SetFinalizer(i, (*Image).Dispose) + runtime.SetFinalizer(i, (*Image).disposeFromFinalizer) return i }