Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal/xgbin: remove use of most of the calls to binary.Read #15

Closed
wants to merge 1 commit into from

Conversation

sbinet
Copy link

@sbinet sbinet commented Sep 19, 2018

This CL gets rid of most of the calls to binary.Read which makes heavy
use of reflection.

Also, drop bufio.Reader in favor of io.Reader.

Fixes #13.

This CL gets rid of most of the calls to binary.Read which makes heavy
use of reflection.

Also, drop bufio.Reader in favor of io.Reader.

Fixes dmitryikh#13.
@coveralls
Copy link

coveralls commented Sep 19, 2018

Pull Request Test Coverage Report for Build 52

  • 31 of 52 (59.62%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 60.07%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/xgbin/xgbin_io.go 31 52 59.62%
Totals Coverage Status
Change from base Build 51: -0.1%
Covered Lines: 689
Relevant Lines: 1147

💛 - Coveralls

@dmitryikh
Copy link
Owner

dmitryikh commented Sep 19, 2018

@sbinet , thanks for your help to the project.

I did some benchmark on loading speed:

func BenchmarkHiggsLoading(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_, err := XGEnsembleFromFile(filepath.Join("testdata", "xghiggs.model"))
		if err != nil {
			b.Skip(err.Error())
		}
	}
}
[leaves] go test -bench Loading
goos: darwin
goarch: amd64
pkg: github.com/dmitryikh/leaves
BenchmarkHiggsLoading-8   	       5	 207094885 ns/op   <--- after removing binary.Read
PASS
ok  	github.com/dmitryikh/leaves	2.179s
[leaves] git rebase -i HEAD~2
Stopped at b724c93...  [+] xgboost model loading benchmark
...
[leaves] go test -bench Loading
goos: darwin
goarch: amd64
pkg: github.com/dmitryikh/leaves
BenchmarkHiggsLoading-8   	       5	 213136769 ns/op    <--- before removing binary.Read
PASS
ok  	github.com/dmitryikh/leaves	2.177s

As you can see the speedup is around ~4%

If you wish you can repeat the experiment taking xghiggs.model from:
https://github.com/dmitryikh/leaves/releases/tag/testdata_v2

Your PR seems nice to me. And if you don't want to add something, I will merge it.

@sbinet
Copy link
Author

sbinet commented Sep 19, 2018

that's a bit disappointing... :)

there's probably some more perf to squeeze out of ReadStruct.
also, right now ReadFloat32Slice is only used in one place where we just discard the newly allocated []float32. Perhaps we could just discard it up-front instead of creating it and then discarding it...

@dmitryikh
Copy link
Owner

@sbinet, I did some investigations:

  1. ReadFloat32Slices is called vary rarely (once per tree if SizeLeafVector > 0 which is false in the benchmark)
  2. ReadInt32Slice is called once for model.
  3. Most (almost all time) we seats in the ReadStruct function:
[leaves] go tool pprof  leaves.test cpu.out
File: leaves.test
Type: cpu
Time: Sep 21, 2018 at 7:21am (MSK)
Duration: 1.41s, Total samples = 1.28s (90.88%)
Entering interactive mode (type "help" for commands, "o" for options)
(pprof) list ReadStruct
Total: 1.28s
ROUTINE ======================== github.com/dmitryikh/leaves/internal/xgbin.ReadStruct in /Users/d.khominich/code/go/src/github.com/dmitryikh/leaves/internal/xgbin/xgbin_io.go
         0      1.13s (flat, cum) 88.28% of Total
         .          .    134:	NameGbm string
         .          .    135:}
         .          .    136:
         .          .    137:// ReadStruct - read arbitrary data structure from binary stream
         .          .    138:func ReadStruct(reader *bufio.Reader, dst interface{}) error {
         .      1.13s    139:	err := binary.Read(reader, binary.LittleEndian, dst)
         .          .    140:	if err != nil {
         .          .    141:		return err
         .          .    142:	}
         .          .    143:	return nil
         .          .    144:}

And memory allocations:

(pprof) list ReadStruct
Total: 943.14MB
ROUTINE ======================== github.com/dmitryikh/leaves/internal/xgbin.ReadStruct in /Users/d.khominich/code/go/src/github.com/dmitryikh/leaves/internal/xgbin/xgbin_io.go
         0   445.01MB (flat, cum) 47.18% of Total
         .          .    134:	NameGbm string
         .          .    135:}
         .          .    136:
         .          .    137:// ReadStruct - read arbitrary data structure from binary stream
         .          .    138:func ReadStruct(reader *bufio.Reader, dst interface{}) error {
         .   445.01MB    139:	err := binary.Read(reader, binary.LittleEndian, dst)
         .          .    140:	if err != nil {
         .          .    141:		return err
         .          .    142:	}
         .          .    143:	return nil
         .          .    144:}

Do you have any ideas how to speedup ReadStruct? Is it possible to read just sizeof(SomeStructType) raw bytes and then cast them to SomeStructType object?

@sbinet
Copy link
Author

sbinet commented Sep 21, 2018

I am in the process of introducing a 'Decoder' type (like for json) that should deal with these pesky structs.
Stay tuned.

@dmitryikh
Copy link
Owner

out of date

@dmitryikh dmitryikh closed this Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants