Skip to content

Conversation

@yevgenypats
Copy link
Contributor

This makes sure every CQType implements size so we can report to the users also how many bytes/MB were synced in addition to number of resources/rows.

This will also be useful in our analytics.

@github-actions
Copy link

github-actions bot commented Dec 17, 2022

⏱️ Benchmark results

  • DefaultConcurrencyDFS-2 resources/s: 10,775
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,489
  • Glob-2 ns/op: 204.9
  • TablesWithChildrenDFS-2 resources/s: 28,534
  • TablesWithChildrenRoundRobin-2 resources/s: 25,521
  • TablesWithRateLimitingDFS-2 resources/s: 28.3
  • TablesWithRateLimitingRoundRobin-2 resources/s: 819.5
  • BufferedScanner-2 ns/op: 12.56
  • LogReader-2 ns/op: 37.91

Copy link
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Could we use https://github.com/DmitriyVTitov/size for this?
It seems calculating the size in bytes of values has a lot of gotchas, so maybe better to use a library that already has a few tests for it?

If not using a library, maybe use reflect.Size() https://go.dev/play/p/Sogem09ks3o

@yevgenypats
Copy link
Contributor Author

Could we use https://github.com/DmitriyVTitov/size for this? It seems calculating the size in bytes of values has a lot of gotchas, so maybe better to use a library that already has a few tests for it?

If not using a library, maybe use reflect.Size() https://go.dev/play/p/Sogem09ks3o

We don't want to use https://github.com/DmitriyVTitov/size/blob/master/size.go as this uses reflection. There is no much gotchas for size calculation but also we mostly use it for a ballpark number it doesn't has to be the exact number of bytes. To have the exact number of bytes we need to marshal it and do len. If we will ever need it to be byte accurate then we can change to marshal and len approach but it will introduce overhead

}

func (dst *Int8Array) Size() int {
return 8 * len(dst.Elements)
Copy link
Member

Choose a reason for hiding this comment

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

Can we call Int8.Size() here?

Copy link
Member

Choose a reason for hiding this comment

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

I think this one is probably okay, the name even has 8 in it, and we're unlikely to redefine the size of 64-bit ints :) Otherwise we have to write:

if len(dst.Elements) == 0 {
     return 0
}
return dst.Elements[0] * len(dst.Elements)

And then there's still the assumption that all the elements have the same size.

We could also do:

	totalSize := 0
	for _, element := range dst.Elements {
		totalSize += element.Size()
	}
	return totalSize

but that would be unnecessarily inefficient, being O(N) instead of O(1)

@erezrokah
Copy link
Member

To have the exact number of bytes we need to marshal it and do len. If we will ever need it to be byte accurate then we can change to marshal and len approach but it will introduce overhead

Marshal also uses reflection https://cs.opensource.google/go/go/+/refs/tags/go1.19.4:src/encoding/json/encode.go;l=330;drc=0b2ad1d815ea8967c49b32d848b2992d0c588d88.

I think the question is what we want, size in memory, or serialized size.

@hermanschaaf hermanschaaf requested a review from erezrokah January 5, 2023 14:05
@kodiakhq kodiakhq bot merged commit 7c15d9a into main Jan 6, 2023
@kodiakhq kodiakhq bot deleted the feat/type_sizes branch January 6, 2023 09:05
kodiakhq bot pushed a commit that referenced this pull request Jan 9, 2023
🤖 I have created a release *beep* *boop*
---


## [1.22.0](v1.21.0...v1.22.0) (2023-01-06)


### Features

* Add size in bytes to CQ types ([#510](#510)) ([7c15d9a](7c15d9a))
* Add WithIgnoreInTestsTransformer ([#579](#579)) ([f836abd](f836abd))
* Add WithResolverTransformer ([#578](#578)) ([5aeba0e](5aeba0e))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants