Skip to content
This repository has been archived by the owner on Jan 22, 2022. It is now read-only.

WIP: Split SWbemServices initialization out of Query method #23

Merged
merged 1 commit into from
Mar 1, 2017

Conversation

gbrayut
Copy link
Contributor

@gbrayut gbrayut commented Feb 9, 2017

I am starting to refactor the WMI/OLE initialization code out of the hot path. I talked with Craig and the current approach is going to be using a channel to send incoming query requests to a dedicated go routine that re-uses the same SWbemServices object instead of creating a new one each time. This should prevent the memory leaks we have seen on Server 2016/Windows 10 and give a good performance boost.

Goals:

  1. Stop leaking memory on Server 2016/Windows 10. Current implementation leaks ~1.5MB of private working set memory for every 10,000 calls (see go test -run TestWbemMemory -timeout 60m )
  2. Only call ole.CoInitializeEx once (call runtime.LockOSThread before and then only unlock on close)
  3. In general we should try and be forgiving against any issues with the OLE/COM/WMI subsystems. We occasionally see things stop working with various error messages, so defensive coding is likely to make things work better in the long run.
  4. Don't break the existing wmi.Query method signature. While testing we will call a function to switch scollector to use the background processing mode. After we verify that it works we can either remove the old code or just default to the background mode.
  5. Strech Goal: the package could allow multiple SWbemServices instances if someone wants to have multiple background processing threads.

Work Items:

  • Create new type and move WMI/OLE initialization logic into a factory method
  • Add new Query method that sends request to the dedicated background processor and returns results via channel/callback.
  • Find a way to report errors on closing back to the caller. May also need to recover from panics on Release/Clear calls when closing.
  • Verify that private memory usage does not grow over time (TestWbemMemory with x00,000 iterations should remain below 10MB).
  • Compare performance of old and new methods using go test benchmarks
  • If SWbemServicesClient on wmi.DefaultClient is not nil use that instead of the old query code.
  • Test how much faster the code is if the ConnectServer call is not in the hot path. This would require initializing multiple background workers (one for each of the different ConnectServer arguments). It may be worth having a pre-connected background worker that is used whenever the arguments are nil (meaning the default namespace is used).

Unknowns:

  1. We may need to figure out under what conditions we need to re-initialize the SWbemServices object. Things like remote queries can definitely fail, but even local queries could stop working and require re-initialization.
    • A method for explicit re-initialization may be required, or the user can just close the current instance and create a new one.
  2. No idea how long this will take, but we plan on testing extensively with scollector prior to merging anything to master here.

@tlimoncelli
Copy link
Contributor

I don't know if this is relevant but have you considered sync.Once?

See http://marcio.io/2015/07/singleton-pattern-in-go/.

@gbrayut
Copy link
Contributor Author

gbrayut commented Feb 9, 2017

Thanks for the link! In this case I don't think we need to worry about thread safety or sync/locks as the creation of the default instance can be managed in the init function.

@gbrayut
Copy link
Contributor Author

gbrayut commented Feb 10, 2017

Finished with the first draft of SWbemServices worker. I left in a bunch of print statements to show the code execution path. @captncraig Ready for a code review tomorrow AM (I'll be out tomorrow afternoon) or on Monday.

C:\Code\Go\src\github.com\stackexchange\wmi [gbray]> go test -run TestWbemQuery
InitializeSWbemServices: Starting
process: starting background thread initialization
process: initialized. closing initError
process: waiting for queries
InitializeSWbemServices: Finished
Query: Sending query request
process: new query: len(query)=773
queryBackground: Starting
queryBackground: Finished
process: s.queryBackground finished
Query: Finished
Query: Sending query request
process: new query: len(query)=775
queryBackground: Starting
queryBackground: Finished
process: s.queryBackground finished
Query: Finished
Close: sending close request
process: queries channel closed
closeBackground: Starting
closeBackground: Finished
Close: finished
PASS
ok      github.com/stackexchange/wmi    0.642s

swbemservices.go Outdated
return err //Send error to caller
}
break WaitForFinishedSignal
default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove default case. This makes a busy loop and burns cpu while waiting.

swbemservices.go Outdated
type queryRequest struct {
query string
dst interface{}
args interface{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be []interface{}

swbemservices.go Outdated
return nil, err //Send error to caller
}
break WaitForProcessSignal
default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No default case here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you also don't need the for/selects in these loops. Most of them look like a single channel read.

swbemservices.go Outdated
close(s.closeError)
}
}
default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No default anywhere.

swbemservices.go Outdated
fmt.Println("Close: sending close request")
var result error
ce := make(chan error)
s.closeError = ce //Race condition if multiple callers to close. May need to lock here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. a lock for close would be good. Make sure to check queries after you get the lock as well. It could be closed / nil.

swbemservices.go Outdated
}

// This method will be called on the background thread and must clean up all WMI/OLE and chan resources
func (s *SWbemServices) closeBackground() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you defer in the process function, closeBackground is unnecessary.

swbemservices.go Outdated
fmt.Println("process: initialized. closing initError")
close(initError)
fmt.Println("process: waiting for queries")
for {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    for q := range queries {
        //got a query
    }
    //closed

swbemservices.go Outdated
if s == nil || s.sWbemLocatorIDispatch == nil {
return fmt.Errorf("SWbemServices is not Initialized")
}
if s.queries == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible race condition here. If it closes between the nil check and putting stuff on the channel. I'd make a single lock and you need to acquire it to insert a query or close. Since everything queues up anyway, it shouldn't be a problem.

@gbrayut
Copy link
Contributor Author

gbrayut commented Feb 14, 2017

@captncraig I made the requested changes and added a benchmark. Looks like for a simple query like Win32_OperatingSystem the new method takes ~53ms and the old method takes ~63ms.

C:\Code\Go\src\github.com\stackexchange\wmi [gbray +0 ~2 -0]> go test -run=NONE -bench=Version -benchtime=120s
BenchmarkNewVersion-8               3000          53435903 ns/op
BenchmarkOldVersion-8               3000          63050507 ns/op
PASS
ok      github.com/stackexchange/wmi    360.940s

@gbrayut
Copy link
Contributor Author

gbrayut commented Mar 1, 2017

Haven't found any issues while testing in Scollector, so merged these to master

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants