Skip to content
This repository has been archived by the owner on Aug 29, 2020. It is now read-only.

Adding OSX support #27

Closed
3 tasks done
cjbassi opened this issue Apr 29, 2018 · 26 comments
Closed
3 tasks done

Adding OSX support #27

cjbassi opened this issue Apr 29, 2018 · 26 comments
Labels
bug Something isn't working enhancement New feature or request help wanted Extra attention is needed

Comments

@cjbassi
Copy link
Owner

cjbassi commented Apr 29, 2018

Need to fix the following issues caused by gopsutil on OSX:

  • temperatures widget doesn't work
  • CPU percentages only work when building from source (cross compiling doesn't work)
  • getting process info takes a long time and is CPU intensive

I don't have a Mac to test on so help is appreciated.

@cjbassi cjbassi added bug Something isn't working enhancement New feature or request help wanted Extra attention is needed labels Apr 29, 2018
@f1337
Copy link
Contributor

f1337 commented Apr 29, 2018

I have a Mac and I am willing to help:

@cjbassi
Copy link
Owner Author

cjbassi commented Apr 29, 2018

@f1337 Thanks! I just added links to the issues for the CPU percents and process info which should help. Let me know if there's anything else you need.

Also, I haven't determined why the temperatures widget doesn't work and I haven't opened an issue for it yet either. Temperature info is gathered here so checking the output or seeing if it just runs slow would be good.

@shirou
Copy link

shirou commented Apr 29, 2018

sorry, temperatures feature on Darwin has been removed due to mistakenly-included GPL code (see 518)

@f1337
Copy link
Contributor

f1337 commented Apr 29, 2018

for the temperatures widget: gotop is GPL, so i will try to reinstate the code that was removed from goposutil, within this project.

@f1337
Copy link
Contributor

f1337 commented Apr 29, 2018

re: cross-compiling, one of these solution may be helpful:

(this is mostly a note-to-self)

@f1337
Copy link
Contributor

f1337 commented Apr 29, 2018

re: process list performance, maybe i'm mis-reading the source, but it looks like syscall is already being used? shirou/gopsutil#513 (comment)

in the meantime, i'm testing compiling against go 1.9 and 1.8, to see if that has any effect.

UPDATE: Go 1.8 and 1.9 had no noticeable effect on performance on my mac.

UPDATE 2: in theory, it is possible that cgo itself is to blame for the performance issues. messaging betw Go and C isn't free. still investigating.

@cjbassi
Copy link
Owner Author

cjbassi commented Apr 29, 2018

@f1337 xgo looks interesting. I can try building a binary using that and have you test it at some point. I also didn't know about the CGO_ENABLED env variable used at build time either, so I can try that too.

Migrating away from using cgo in gopsutil would also be good, since they only use it for Darwin and it seems like a goal for them to not use it at all. Dunno how long/difficult that would be.

@f1337
Copy link
Contributor

f1337 commented Apr 29, 2018

xgo runs in a Docker container, so its "native" OS is linux. I'm testing locally right now, should have some results shortly. :)

I'm not sure the CGO_ENABLED flag will actually work if goreleaser isn't run on a darwin machine. :(

re: gopsutil moving away from cgo, the issue appears to be that CPU times via the Mach kernel require C libs: shirou/gopsutil#66. The previous solution involved compiling a binary helper for Darwin installs, which is arguably worse. 🤷‍♂️

@f1337
Copy link
Contributor

f1337 commented Apr 29, 2018

@cjbassi I am happy to test any binaries you build. 👍

re: xgo, I think we have a winner! 🎉 The resulting ./gotop-darwin-10.6-amd64 worked just as well as when I manually compiled with go get.

xgo output:

go/bin/xgo --targets="darwin/*" github.com/cjbassi/gotop
Checking docker installation...
Client:
 Version:      18.03.1-ce
 API version:  1.37
 Go version:   go1.9.5
 Git commit:   9ee9f40
 Built:        Thu Apr 26 07:13:02 2018
 OS/Arch:      darwin/amd64
 Experimental: false
 Orchestrator: swarm

Server:
 Engine:
  Version:      18.03.1-ce
  API version:  1.37 (minimum version 1.12)
  Go version:   go1.9.5
  Git commit:   9ee9f40
  Built:        Thu Apr 26 07:22:38 2018
  OS/Arch:      linux/amd64
  Experimental: true

Checking for required docker image karalabe/xgo-latest... found.
Cross compiling github.com/cjbassi/gotop...
Fetching main repository github.com/cjbassi/gotop...
github.com/cjbassi/gotop (download)
Compiling for darwin-10.6/amd64...
Compiling for darwin-10.6/386...
Cleaning up build environment...

Note the difference in size between the generated binaries:

ls -al dist/darwin_amd64/
-rwxr-xr-x  1 me  staff  2799140 Apr 29 13:47 gotop
ls -al gotop-darwin-10.6-amd64
-rwxr-xr-x  1 me  staff  3044844 Apr 29 13:47 gotop-darwin-10.6-amd64

xgo installation:

docker pull karalabe/xgo-latest
go get github.com/karalabe/xgo

@cjbassi
Copy link
Owner Author

cjbassi commented Apr 29, 2018

Wow, very awesome and building for me was a breeze. 👌

Here's the 64bit binary in a zip file for you to test again (didn't know if there's a better way to upload this):
gotop-darwin-10.6-amd64.zip

@f1337
Copy link
Contributor

f1337 commented Apr 29, 2018

Yours worked for me! Still the 12s spin-up time, but no crashing! 🎉

I am working on a pull-request that adds a bash script for compiling via xgo to the goreleaser post-build-hook. I have tested it on my own fork, and it's working nicely (needs a wee bit of cleanup). Here is the release I just created, with a working Darwin build via xgo: https://github.com/f1337/gotop/releases/tag/1.2.15

@f1337
Copy link
Contributor

f1337 commented Apr 30, 2018

re: temperatures => we are using an older version of gopsutil, which still has the GPL'd code. Our problem is that the temperature code in gotop is ignoring sensors that don't have "input" in the name, and MacOS sensors don't conform to that pattern:

%#v[{"sensorKey":"TA0P","sensorTemperature":0} {"sensorKey":"TA1P","sensorTemperature":0} {"sensorKey":"TC0D","sensorTemperature":0} {"sensorKey":"TC0H","sensorTemperature":0} {"sensorKey":"TC0P","sensorTemperature":59} {"sensorKey":"TB0T","sensorTemperature":32} {"sensorKey":"TB1T","sensorTemperature":32} {"sensorKey":"TB2T","sensorTemperature":31} {"sensorKey":"TB3T","sensorTemperature":0} {"sensorKey":"TG0D","sensorTemperature":64} {"sensorKey":"TG0H","sensorTemperature":0} {"sensorKey":"TG0P","sensorTemperature":59} {"sensorKey":"TH0P","sensorTemperature":0} {"sensorKey":"TM0S","sensorTemperature":50} {"sensorKey":"TM0P","sensorTemperature":51} {"sensorKey":"TN0H","sensorTemperature":0} {"sensorKey":"TN0D","sensorTemperature":0} {"sensorKey":"TN0P","sensorTemperature":0} {"sensorKey":"TI0P","sensorTemperature":0} {"sensorKey":"TI1P","sensorTemperature":0} {"sensorKey":"TW0P","sensorTemperature":44}]

The sensor keys for MacOS/Darwin can be seen here: shirou/gopsutil@c95755e?diff=unified#diff-519d82962db93621287a0d1ff038f5c6L67

Of course, now that gopsutil has dropped Apple's GPL'd code, we'll also need to re-introduce it to gotop, one way or another.

@f1337
Copy link
Contributor

f1337 commented Apr 30, 2018

re: startup performance, my debugging above demonstrated that during the ~12 seconds gotop takes to start on MacOS, the temp widget's update() method is called every second. We may be able to get some quick startup-performance gains by ensuring the widgets aren't calling update() until all are ready. I'll keep digging.

@cjbassi
Copy link
Owner Author

cjbassi commented Apr 30, 2018

The slow startup time is actually just a side effect of process info retrieval taking a really long time on Mac. Gotop isn't loaded until each widget is loaded and updated with info, which isn't a problem on Linux but takes a long time on Mac because it takes ~12 second to update process info. However, we're supposed to be updating process info every second too.

As far as getting the info goes, the issue isn't actually with the process.Processes() function (which uses syscalls as you pointed out), but with the functions that are called on each Process returned by that function, which you can see here:

gotop/widgets/proc.go

Lines 77 to 81 in 8e750b8

for i, psProcess := range psProcesses {
pid := psProcess.Pid
command, _ := psProcess.Name()
cpu, _ := psProcess.CPUPercent()
mem, _ := psProcess.MemoryPercent()

I tried getting process info in parallel using goroutines to speed things up, but that just put CPU usage at 100%. So it's an issue of CPU usage and speed.

Hope that makes sense XD

edit: It would probably be good to play around and see which of the 3 Process methods are the culprit (I'm guessing its CPUPercent)

@f1337
Copy link
Contributor

f1337 commented May 6, 2018

I did some logging on my mac, and the results are interesting:

  • Each psProcess in the loop takes ~46ms to inspect: ~30ms for CPUPercent(), and ~15ms for MemoryPercent().
  • Total time to inspect all ~300 processes is ~12s (that's full seconds, which is roughly equivalent to 300 * 46ms).
  • When I removed the calls to CPUPercent() and MemoryPercent(), total time to inspect all ~300 processes dropped to 8ms (yes, milliseconds).

proc_startup.log
proc_startup_no_cpu_mem.log

Unrelated to performance, I also noticed some interesting discrepancies w/ Activity Monitor:

  • psProc.Processes() returns 251 kernel_task processes, while Activity Monitor shows 160 kernel_task threads. top and htop don't report any kernel_task processes at all.
  • psProc.Processes() returns 50 unnamed processes, which have no obvious relationship to any processes I see in Activity Monitor, top or htop.

@cjbassi
Copy link
Owner Author

cjbassi commented May 6, 2018

Very nice, thanks! Can I ask how you generated those logs?

So the issues with CPUPercent and MemoryPercent make sense, since gopsutil is calling the ps command, once with MemoryPercent and twice with CPUPercent. So that has to be fixed somehow, possibly with syscalls. Gonna look into syscalls, and maybe check out how other libraries monitor cpu usage too.

@f1337
Copy link
Contributor

f1337 commented May 6, 2018

To generate the logs, I mangled Proc.update() as shown below. You'll want to import "github.com/ccding/go-logging/logging". It will output to a file called logging.log.

func (self *Proc) update() {
	psProcesses, _ := psProc.Processes()
	processes := make([]Process, len(psProcesses))

	logger, _ := logging.BasicLogger("dummylog")
	logger.Error("Proc.update() ================================================================================")

	allStart := time.Now()

	for i, psProcess := range psProcesses {
		processStart := time.Now()

		start := time.Now()
		pid := psProcess.Pid
		elapsed := time.Since(start)
		logger.Error("psProcess.Pid took %s", elapsed)

		start = time.Now()
		command, _ := psProcess.Name()
		elapsed = time.Since(start)
		logger.Error("psProcess.Name() took %s", elapsed)

		start = time.Now()
		cpu, _ := psProcess.CPUPercent()
		elapsed = time.Since(start)
		logger.Error("psProcess.CPUPercent() took %s", elapsed)
		
		start = time.Now()
		mem, _ := psProcess.MemoryPercent()
		elapsed = time.Since(start)
		logger.Error("psProcess.MemoryPercent() took %s", elapsed)

		start = time.Now()
		processes[i] = Process{
			pid,
			command,
			0,
			0,
		}
		elapsed = time.Since(start)
		logger.Error("Process{} took %s", elapsed)

		processElapsed := time.Since(processStart)
		logger.Error("%s process total inspection time %s", command, processElapsed)
		logger.Error("----------------------------------------------------------------------------------------------")
	}

	allElapsed := time.Since(allStart)
	logger.Error("All processes total inspection time %s", allElapsed)
	logger.Error("**********************************************************************************************")
	logger.Destroy()

	self.ungroupedProcs = processes
	self.groupedProcs = Group(processes)

	self.Sort()
}

@cjbassi
Copy link
Owner Author

cjbassi commented May 15, 2018

Alright I think I've made a bit of progress on things. 😅

For the processes issue, I've actually switched from using gopsutil to calling ps on both Linux and Darwin for several reasons documented in the commit message. It's working nicely on Linux but still needs to be tested on Darwin. It should fix the issue on Darwin since we're calling ps once per interval instead of once per process.

For the temperatures issue, I've made a branch called 'temps' that needs to be tested on Darwin. I basically updated gopsutil through dep, added back the gpl code that was removed, and then made separate files for the temperature widget's update method for each platform.

@f1337
Copy link
Contributor

f1337 commented May 23, 2018

I'll test the latest on Darwin this weekend. Apologies for the delayed replies the past couple weeks, I've been too busy to do anything fun.

@f1337
Copy link
Contributor

f1337 commented May 23, 2018

🎉 It works! That is, no 12-or-more seconds delay on startup. I had to make one change on line #27 of proc_unix.go, dropping the --no-headers switch to ps b/c macOS does not support it:

output, _ := exec.Command("ps", "-axo", "pid,comm,pcpu,pmem,args").Output()

@cjbassi
Copy link
Owner Author

cjbassi commented May 23, 2018

Great! I just fixed that one line in master. What about temperatures? Are those working/did you test the temp branch?

@f1337
Copy link
Contributor

f1337 commented May 24, 2018

with one minor typo fix, temps work: #36

:)

@cjbassi
Copy link
Owner Author

cjbassi commented May 24, 2018

I merged everything and got some releases up some hopefully everything is working now!

Could you try installing gotop using homebrew, making sure it works, and making sure the version number is 1.4.0? Install through Homebrew with:

brew tap cjbassi/gotop
brew install gotop

@f1337
Copy link
Contributor

f1337 commented May 24, 2018

💥 no startup log, temps work, processes work, via hombrew:

screen shot 2018-05-24 at 5 10 41 pm

@cjbassi
Copy link
Owner Author

cjbassi commented May 24, 2018

Yay! Good to see. Ran into some issues with goreleaser and xgo but I'm glad that it's working. Just wanted to say thanks for the troubleshooting, testing, and PR's and that it's been fun getting this to work! XD Gonna add you to the Credits section of the readme cause I appreciate it.

@cjbassi cjbassi closed this as completed May 24, 2018
@f1337
Copy link
Contributor

f1337 commented May 25, 2018

awesome, you're welcome, and thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants