StackExchange / wmi

WMI for Go
http://godoc.org/github.com/StackExchange/wmi
MIT License
433 stars 173 forks source link

WIP: Split SWbemServices initialization out of Query method #23

Closed gbrayut closed 7 years ago

gbrayut commented 7 years ago

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:

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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
gbrayut commented 7 years ago

@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 commented 7 years ago

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