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

Iterable ordered map alternative with improved performance #1152

Merged

Conversation

hanjm
Copy link
Contributor

@hanjm hanjm commented Dec 3, 2023

Summary

fix #1151, introduce a v2 interface for type assert.

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added

@hanjm hanjm force-pushed the feature/jimmiehan-ordered-map-performance branch from 94567db to 39084fe Compare December 3, 2023 01:16
@hanjm hanjm changed the title Improve ordered map performance (#1151) Improve ordered map performance Dec 3, 2023
@hanjm hanjm force-pushed the feature/jimmiehan-ordered-map-performance branch from 39084fe to 2c82b79 Compare December 3, 2023 01:51
@timmy21
Copy link

timmy21 commented Dec 7, 2023

maybe use below interface?

type MapIter interface {
	Next() bool
	Key() any
	Value() any
}

type OrderedMapV2 interface {
	Put(key any, value any)
	Range() MapIter
}

so we can use more efficient way to store map, for example: two slices

type Pairs struct {
	Keys []any
	Values []any
}

@hanjm
Copy link
Contributor Author

hanjm commented Dec 9, 2023

maybe use below interface?

type MapIter interface {
	Next() bool
	Key() any
	Value() any
}

type OrderedMapV2 interface {
	Put(key any, value any)
	Range() MapIter
}

so we can use more efficient way to store map, for example: two slices

type Pairs struct {
	Keys []any
	Values []any
}

Looks great.

$ CLICKHOUSE_USE_DOCKER=false CLICKHOUSE_PASSWORD=tps go test -gcflags="-l" . -bench OrderedMap -run OrderedMap
using random seed 1702120904860719000 for native tests
goos: darwin
goarch: arm64
pkg: github.com/ClickHouse/clickhouse-go/v2/tests
BenchmarkOrderedMapUseChanGo-10                   584689              2023 ns/op             120 B/op          2 allocs/op
BenchmarkOrderedMapKeysUseChanNoGo-10            2493792               502.1 ns/op           256 B/op          2 allocs/op
BenchmarkOrderedMapKeysUseSlice-10              11242239               123.8 ns/op             0 B/op          0 allocs/op
BenchmarkOrderedMapKeysUseIter-10               61061322                19.83 ns/op           16 B/op          1 allocs/op
PASS
ok      github.com/ClickHouse/clickhouse-go/v2/tests    7.274s

@hanjm hanjm force-pushed the feature/jimmiehan-ordered-map-performance branch from 6a00681 to 00729d0 Compare December 9, 2023 11:34
@timmy21
Copy link

timmy21 commented Dec 9, 2023

you can also benchmark reflect map

func BenchmarkReflectMapIter(b *testing.B) {
	m := make(map[int]int)
	for i := 0; i < 10; i++ {
		m[i] = i
	}
	b.ResetTimer()
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		value := reflect.Indirect(reflect.ValueOf(m))
		iter := value.MapRange()
		for iter.Next() {
			_ = iter.Key().Interface()
			_ = iter.Value().Interface()
		}
	}
}

tests/map_test.go Outdated Show resolved Hide resolved
tests/map_test.go Outdated Show resolved Hide resolved
tests/map_test.go Outdated Show resolved Hide resolved
@hanjm
Copy link
Contributor Author

hanjm commented Dec 9, 2023

add benchmark reflect map.

$ CLICKHOUSE_USE_DOCKER=false CLICKHOUSE_PASSWORD=tps go test -gcflags="-l" . -bench OrderedMap -run OrderedMap
using random seed 1702126424636270000 for native tests
goos: darwin
goarch: arm64
pkg: github.com/ClickHouse/clickhouse-go/v2/tests
BenchmarkOrderedMapUseChanGo-10                   600756              1970 ns/op             120 B/op          2 allocs/op
BenchmarkOrderedMapKeysUseChanNoGo-10            2467634               483.4 ns/op           256 B/op          2 allocs/op
BenchmarkOrderedMapKeysUseSlice-10              11058926               110.1 ns/op             0 B/op          0 allocs/op
BenchmarkOrderedMapKeysUseIter-10               20183755                57.78 ns/op           16 B/op          1 allocs/op
BenchmarkOrderedMapReflectMapIter-10             1732650               697.0 ns/op           448 B/op         21 allocs/op
PASS
ok      github.com/ClickHouse/clickhouse-go/v2/tests    8.459s

@hanjm hanjm force-pushed the feature/jimmiehan-ordered-map-performance branch from 2d4f09a to 1b9a601 Compare December 9, 2023 13:01
@jkaflik
Copy link
Contributor

jkaflik commented Dec 11, 2023

@hanjm thanks for your contribution! Before I jump into review, could you elaborate on this implementation please?

@timmy21
Copy link

timmy21 commented Dec 12, 2023

@hanjm thanks for your contribution! Before I jump into review, could you elaborate on this implementation please?

The problem I ran into was that when writing to map columns, a lot of time was wasted in the reflect operation
Screen Shot 2023-12-12 at 12 29 23 PM

@jkaflik
Copy link
Contributor

jkaflik commented Dec 12, 2023

@timmy21 I understand the reasoning, but I want to understand what is your proposal to solve this.

@timmy21
Copy link

timmy21 commented Dec 12, 2023

@timmy21 I understand the reasoning, but I want to understand what is your proposal to solve this.

By defining a map interface, this allows user to store map using different data structure, so that avoiding the reflect overhead and improve current OrderedMap interface performance. for example use Pairs:

type Pairs struct {
	Keys []any
	Values []any
}

Copy link
Contributor

@jkaflik jkaflik left a comment

Choose a reason for hiding this comment

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

Overall it looks good. Only minor naming convention changes.

Would be great if we provide additional example.

lib/column/map.go Outdated Show resolved Hide resolved
lib/column/map.go Outdated Show resolved Hide resolved
@hanjm hanjm force-pushed the feature/jimmiehan-ordered-map-performance branch from 02623c4 to c596f39 Compare December 13, 2023 11:03
@hanjm hanjm force-pushed the feature/jimmiehan-ordered-map-performance branch from c596f39 to 9a0d0d5 Compare December 13, 2023 11:09
@hanjm
Copy link
Contributor Author

hanjm commented Dec 13, 2023

Overall it looks good. Only minor naming convention changes.

Would be great if we provide additional example.

@jkaflik, Thanks, done.

@jkaflik jkaflik changed the title Improve ordered map performance Iterable ordered map alternative with improved performance Dec 13, 2023
@jkaflik jkaflik merged commit 9d8573d into ClickHouse:main Dec 13, 2023
13 checks passed
mx-psi referenced this pull request in open-telemetry/opentelemetry-collector-contrib Jan 3, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[github.com/ClickHouse/clickhouse-go/v2](https://togithub.com/ClickHouse/clickhouse-go)
| `v2.16.0` -> `v2.17.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.16.0/v2.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.16.0/v2.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>ClickHouse/clickhouse-go
(github.com/ClickHouse/clickhouse-go/v2)</summary>

###
[`v2.17.0`](https://togithub.com/ClickHouse/clickhouse-go/blob/HEAD/CHANGELOG.md#v2170-2023-12-21----Release-notes-generated-using-configuration-in-githubreleaseyml-at-main---)

[Compare
Source](https://togithub.com/ClickHouse/clickhouse-go/compare/v2.16.0...v2.17.0)

#### What's Changed

##### Enhancements 🎉

- Iterable ordered map alternative with improved performance by
[@&#8203;hanjm](https://togithub.com/hanjm) in
[https://github.com/ClickHouse/clickhouse-go/pull/1152](https://togithub.com/ClickHouse/clickhouse-go/pull/1152)
- Support bool alias type by
[@&#8203;yogasw](https://togithub.com/yogasw) in
[https://github.com/ClickHouse/clickhouse-go/pull/1156](https://togithub.com/ClickHouse/clickhouse-go/pull/1156)

##### Fixes 🐛

- Update README - mention HTTP protocol usable only with `database/sql`
interface by [@&#8203;jkaflik](https://togithub.com/jkaflik) in
[https://github.com/ClickHouse/clickhouse-go/pull/1160](https://togithub.com/ClickHouse/clickhouse-go/pull/1160)
- Fix README example for Debugf by
[@&#8203;aramperes](https://togithub.com/aramperes) in
[https://github.com/ClickHouse/clickhouse-go/pull/1153](https://togithub.com/ClickHouse/clickhouse-go/pull/1153)

#### New Contributors

- [@&#8203;yogasw](https://togithub.com/yogasw) made their first
contribution in
[https://github.com/ClickHouse/clickhouse-go/pull/1156](https://togithub.com/ClickHouse/clickhouse-go/pull/1156)
- [@&#8203;aramperes](https://togithub.com/aramperes) made their first
contribution in
[https://github.com/ClickHouse/clickhouse-go/pull/1153](https://togithub.com/ClickHouse/clickhouse-go/pull/1153)

**Full Changelog**:
ClickHouse/clickhouse-go@v2.16.0...v2.17.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMDMuMSIsInVwZGF0ZWRJblZlciI6IjM3LjEwMy4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <[email protected]>
Co-authored-by: Bogdan Drutu <[email protected]>
cparkins referenced this pull request in AmadeusITGroup/opentelemetry-collector-contrib Jan 10, 2024
…-telemetry#30217)

[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[github.com/ClickHouse/clickhouse-go/v2](https://togithub.com/ClickHouse/clickhouse-go)
| `v2.16.0` -> `v2.17.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.16.0/v2.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.16.0/v2.17.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>ClickHouse/clickhouse-go
(github.com/ClickHouse/clickhouse-go/v2)</summary>

###
[`v2.17.0`](https://togithub.com/ClickHouse/clickhouse-go/blob/HEAD/CHANGELOG.md#v2170-2023-12-21----Release-notes-generated-using-configuration-in-githubreleaseyml-at-main---)

[Compare
Source](https://togithub.com/ClickHouse/clickhouse-go/compare/v2.16.0...v2.17.0)

#### What's Changed

##### Enhancements 🎉

- Iterable ordered map alternative with improved performance by
[@&open-telemetry#8203;hanjm](https://togithub.com/hanjm) in
[https://github.com/ClickHouse/clickhouse-go/pull/1152](https://togithub.com/ClickHouse/clickhouse-go/pull/1152)
- Support bool alias type by
[@&open-telemetry#8203;yogasw](https://togithub.com/yogasw) in
[https://github.com/ClickHouse/clickhouse-go/pull/1156](https://togithub.com/ClickHouse/clickhouse-go/pull/1156)

##### Fixes 🐛

- Update README - mention HTTP protocol usable only with `database/sql`
interface by [@&open-telemetry#8203;jkaflik](https://togithub.com/jkaflik) in
[https://github.com/ClickHouse/clickhouse-go/pull/1160](https://togithub.com/ClickHouse/clickhouse-go/pull/1160)
- Fix README example for Debugf by
[@&open-telemetry#8203;aramperes](https://togithub.com/aramperes) in
[https://github.com/ClickHouse/clickhouse-go/pull/1153](https://togithub.com/ClickHouse/clickhouse-go/pull/1153)

#### New Contributors

- [@&open-telemetry#8203;yogasw](https://togithub.com/yogasw) made their first
contribution in
[https://github.com/ClickHouse/clickhouse-go/pull/1156](https://togithub.com/ClickHouse/clickhouse-go/pull/1156)
- [@&open-telemetry#8203;aramperes](https://togithub.com/aramperes) made their first
contribution in
[https://github.com/ClickHouse/clickhouse-go/pull/1153](https://togithub.com/ClickHouse/clickhouse-go/pull/1153)

**Full Changelog**:
ClickHouse/clickhouse-go@v2.16.0...v2.17.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMDMuMSIsInVwZGF0ZWRJblZlciI6IjM3LjEwMy4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <[email protected]>
Co-authored-by: Bogdan Drutu <[email protected]>
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.

OrderedMap interface channel performance overhead
3 participants