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

Optimize NewSafeID using atomic #1035

Merged
merged 5 commits into from Feb 24, 2022
Merged

Conversation

zchee
Copy link
Contributor

@zchee zchee commented Feb 14, 2022

Optimize NewSafeID using atomic instead of a mutex lock.

  • is it performance related

name                  old time/op    new time/op    delta
NewSafeID-20            14.0ns ± 1%     5.9ns ± 1%  -57.76%  (p=0.008 n=5+5)
NewSafeIDParallel-20    23.8ns ± 1%    21.0ns ± 2%  -12.08%  (p=0.008 n=5+5)

name                  old alloc/op   new alloc/op   delta
NewSafeID-20             0.00B          0.00B          ~     (all equal)
NewSafeIDParallel-20     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name                  old allocs/op  new allocs/op  delta
NewSafeID-20              0.00           0.00          ~     (all equal)
NewSafeIDParallel-20      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
name                  old time/op    new time/op    delta
NewSafeID-20            13.9ns ± 2%     7.5ns ± 1%  -46.16%  (p=0.008 n=5+5)
NewSafeIDParallel-20    24.2ns ± 6%    22.1ns ± 1%   -9.06%  (p=0.008 n=5+5)

name                  old alloc/op   new alloc/op   delta
NewSafeID-20             0.00B          0.00B          ~     (all equal)
NewSafeIDParallel-20     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name                  old allocs/op  new allocs/op  delta
NewSafeID-20              0.00           0.00          ~     (all equal)
NewSafeIDParallel-20      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
Copy link
Contributor Author

zchee commented Feb 21, 2022

@kanata2 ping

messageID.go Outdated Show resolved Hide resolved
messageID.go Outdated Show resolved Hide resolved
name                  old time/op    new time/op    delta
NewSafeID-20            7.60ns ± 1%    5.93ns ± 1%  -21.97%  (p=0.008 n=5+5)
NewSafeIDParallel-20    21.0ns ± 1%    21.0ns ± 2%     ~     (p=0.952 n=5+5)

name                  old alloc/op   new alloc/op   delta
NewSafeID-20             0.00B          0.00B          ~     (all equal)
NewSafeIDParallel-20     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name                  old allocs/op  new allocs/op  delta
NewSafeID-20              0.00           0.00          ~     (all equal)
NewSafeIDParallel-20      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

See also:
- slack-go#1035 (review)

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
Copy link
Contributor Author

zchee commented Feb 24, 2022

@brainexe cc: @kanata2 Fixed. PTAL.

Note that @brainexe suggestions will increase more performance. Thanks!

Below is before and after the daa4329 (#1035) commit.
This pull request's actual optimized benchmark result is I'd edited pull request comment.

name                  old time/op    new time/op    delta
NewSafeID-20            7.60ns ± 1%    5.93ns ± 1%  -21.97%  (p=0.008 n=5+5)
NewSafeIDParallel-20    21.0ns ± 1%    21.0ns ± 2%     ~     (p=0.952 n=5+5)

name                  old alloc/op   new alloc/op   delta
NewSafeID-20             0.00B          0.00B          ~     (all equal)
NewSafeIDParallel-20     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name                  old allocs/op  new allocs/op  delta
NewSafeID-20              0.00           0.00          ~     (all equal)
NewSafeIDParallel-20      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

Copy link
Member

@kanata2 kanata2 left a comment

Choose a reason for hiding this comment

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

LGTM.

@brainexe Could you take another look?

@brainexe
Copy link
Contributor

for me it looks good now 👍

@zchee zchee self-assigned this Feb 24, 2022
@zchee
Copy link
Contributor Author

zchee commented Feb 24, 2022

@brainexe Thanks for polite review :D

@kanata2 Can I merge this pull request?

@kanata2
Copy link
Member

kanata2 commented Feb 24, 2022

Yeah, go ahead!

@zchee zchee merged commit f6658aa into slack-go:master Feb 24, 2022
@zchee zchee deleted the opt-new-safe-id branch February 24, 2022 18:29
ghost pushed a commit to range-labs/slack that referenced this pull request Apr 19, 2022
* Change ActionType to public

* Changed type of GetConversationsParameters.ExcludeArchived to bool instead of string.

* Make possible to get authorized user's profile providing empty userID to GetUserProfileContext

* Added coverage for audit API endpoint

* Added all of the other possible entity types for completeness

* Update audit.go

Just cleaning up a comment

* Changed field Ua to UA to better match Go standard

* Add support for "channel_created" in the Events API.

* add thread_ts to container

* restore go.mod and go.sum

* refact: missing attachement fields

* Make UserID optional in GetUserProfile()

`users.profile.get` API takes user parameter as optional.
https://api.slack.com/methods/users.profile.get

This change breaks GetUserProfile interface.
The current interface does not allow the bot to retrieve
its own user profile.

* Handle token_revoked event

This is a critical event and the client should stop trying to reconnect.

Signed-off-by: Andrea Barberio <insomniac@slackware.it>

* Add ReplyTo information to ack error events.

This allows clients to better handle ack errors by linking them
to the original outbound message.

* Make more examples directly runnable

* add the condition to the validation func for TextBlockObject

* Seems the token was missing from the request.

* Token is also added to URL parameters

* Update slackevents/inner_events.go

* Remove deprecated methods

Related issues:

- slack-go#748
- slack-go#876

More details:

- https://api.slack.com/changelog/2020-01-deprecating-antecedents-to-the-conversations-api

* slacktest: Save thread_ts to message

* Add timepicker to UnmarshalJSON

* Fix authorization on methods which uses get requests

* [socketmode] Add methods with passing context

- added socketmode.Client.RunContext
- added socketmode.Client.OpenContext
- changed socketmode.Client.openAndDial to pass context
- added test coverage for socketmode.Client.openAndDial
- added test coverage for socketmode.Client.RunContext
- added test coverage for socketmode.Client.OpenContext

* [socketmode] Use DialContext instead of Dial

* [socketmode] Graceful shutdown websocket.Conn

websocket connection freeze on conn.ReadJSON

- returned context errors to stop reconnect
- closed channel of messages when exit from Client.run
- closed websocket connection when context canceled

* [socketmode] mark test only for go 1.13 and higher

* [socketmode] fix typo in json tag

* Drop support of Go 1.12

* Remove travis badge

* Fix broken examples

* Add support for Go1.16

* [skip-ci] Replace badge

* feat: support response_urls field for view_submission payload

view_submission payload includes response_urls field when specific blocks have
a special parameter(response_url_enabled paramter = true).

See the following Slack docs for more details:

* https://api.slack.com/surfaces/modals/using#modal_response_url
* https://api.slack.com/reference/interaction-payloads/views#view_submission

* add missing token values

* added `external` data_source for dynamic data

`external` kind of input was missing in the slack api
but makes sense to pass this as option rather than separating into
two funcs; also makes sense to pass Optional as option but not hardcoded

* fix: renamed from dynamic to external for dialog input

* slacktest: Fix broken json if newline in text

* support team id param

* Trivial typo fix in the bug reporting template

* Updating audit.go 

Updated audit api code to use the updated getMethod w/ token function

* fix button sample

* fix limit url param

This should be "limit" not "count" according to the current
API docs: https://api.slack.com/admins/audit-logs#monitoring-workspace-events-with-the-audit-logs-api__how-to-call-the-audit-logs-api__audit-logs-endpoints

* Don't include secrets in url

* replace deprecated method

I believe this GetGroups was deleted as a part of this commit:
slack-go@9152ed0

* Rename the member socketmode.Client.apiClient to Client, and be an embedded structure for compatibility

* Fix path

* New events (WIP)

* New events (WIP)

* New events (WIP)

* fix block_context.go doc link

* New events (WIP)

* New events (WIP)

* Restore original paths

* Restore original intents

* Add test setup for Go1.17 environment to CI

* Add Enterprise property

* Fixed LinkSharedEvent and added context aware UnfurlMessage

The MessageTimeStamp was incorrectly marked as a number. It should be
a string as any other timestamp, and it required for Unfurl to work

* Revert conversion to string for now

* Add expires_in and refresh token handling

* Update users example with input examples

* Add Dispatch Action Config to InputBlock

* use pointer

* add TeamJoinEvent

* Update socketmode.go

* webhooks: add additional fields

no issue

These fields allow users to replace original messages sent by bots

eg first sending a ephemeral message and replacing it with one thats in-channel

* add message subType constants from https://api.slack.com/events/message

* Allow wrapping of error metadata. Addresses issue slack-go#939

* Support Rich Text blocks

* Update CHANGELOG.md

* Update bug report template

* fix: don't add API token as a query string in users.setPhoto method

resolve slack-go#992

* sometimes the message_ts isn't a json.Number

* add a new test with the payload from link_shared docs re: unfurls in the message composer

* add a comment for why MessageTimeStamp is a string, not json.Number

* [ci-skip] doc: guide to Slack channel

* [skip-ci] slacktest: fix license issue

Resolved: slack-go#625, slack-go#948, slack-go#979

* add socket mode example link to README

As mentioned in the documentation in https://api.slack.com/rtm:
"For most applications, Socket Mode is a better way to communicate with Slack."

(Personally I got confused between RTM and Socket Mode, and I think this might help future developers)

* Update README.md

* add empty line to be consistent

* add latest_reply property to Msg struct

* introduce workflow step app functionality

* tests for workflowStep added

* example workflowStep app added

* readme improved

* unused variable in example removed, switch line indention to tabulator

* using snake case for new directory and file

* Add refresh_token and token_type to OAuthV2Response fields

* all: add new //go:build lines

$ go version
go version go1.17.7 darwin/amd64

$ make fmt

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* all: remove github.com/pkg/errors dependency

The github.com/pkg/errors package has been deprecated.

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* vendor: run go mod vendor

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* messageID: add benchmark for NewSafeID

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* messageID: optimize NewSafeID using atomic instead of mutex lock

name                  old time/op    new time/op    delta
NewSafeID-20            13.9ns ± 2%     7.5ns ± 1%  -46.16%  (p=0.008 n=5+5)
NewSafeIDParallel-20    24.2ns ± 6%    22.1ns ± 1%   -9.06%  (p=0.008 n=5+5)

name                  old alloc/op   new alloc/op   delta
NewSafeID-20             0.00B          0.00B          ~     (all equal)
NewSafeIDParallel-20     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name                  old allocs/op  new allocs/op  delta
NewSafeID-20              0.00           0.00          ~     (all equal)
NewSafeIDParallel-20      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* messageID: add NewSafeID testcase

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* switch go code style for imports from gofmt to goimports

* all: support pass context.Context to all methods

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* webhooks: remove go1.12 support

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* misc: use NewRequestWithContext

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* misc: use http.MethodXXX constant

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* messageID: fix atomic operation suggested by brainexe

name                  old time/op    new time/op    delta
NewSafeID-20            7.60ns ± 1%    5.93ns ± 1%  -21.97%  (p=0.008 n=5+5)
NewSafeIDParallel-20    21.0ns ± 1%    21.0ns ± 2%     ~     (p=0.952 n=5+5)

name                  old alloc/op   new alloc/op   delta
NewSafeID-20             0.00B          0.00B          ~     (all equal)
NewSafeIDParallel-20     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name                  old allocs/op  new allocs/op  delta
NewSafeID-20              0.00           0.00          ~     (all equal)
NewSafeIDParallel-20      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

See also:
- slack-go#1035 (review)

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* messageID: add documentation

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* chat: add some BuildRequestContext methods for backwards compatibility

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* webhook: remove unnecessary PostWebhookContextCustomHTTP function

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* introduce workflow step app functionality

* tests for workflowStep added

* example workflowStep app added

* readme improved

* unused variable in example removed, switch line indention to tabulator

* using snake case for new directory and file

* switch go code style for imports from gofmt to goimports

* optimize slackutilsx.EscapeMessage function

* workflow_step: add SaveWorkflowStepConfigurationConetxt & fix return err

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* workflow_step: fix typo on SaveWorkflowStepConfigurationContext

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

* github/workflow: drop go1.15 and add go1.18 (slack-go#1048)

* github/workflow: drop go1.15 and add go1.18

* all: run goimports

* WithStyle should be fluent

* Add a fluent WithConfirm for buttons

Co-authored-by: GLOFonseca <guilofonseca@gmail.com>
Co-authored-by: mzduke <support@raidboss.io>
Co-authored-by: Alexander Tunik <2braven@gmail.com>
Co-authored-by: Justin Judd <jbj@google.com>
Co-authored-by: Ward Vandewege <ward@jhvc.com>
Co-authored-by: Ian Hall <ihall@fanatics.com>
Co-authored-by: xnok <nokwebspace@gmail.com>
Co-authored-by: Takuya Kosugiyama <re@itkq.jp>
Co-authored-by: Andrea Barberio <insomniac@slackware.it>
Co-authored-by: Nolan Lum <nolan@nolm.name>
Co-authored-by: David Parsley <parsley@linuxjedi.org>
Co-authored-by: sryoya <sryoya0814@gmail.com>
Co-authored-by: Naoki Kanatani <k12naoki@gmail.com>
Co-authored-by: arran <a.ubels@base2services.com>
Co-authored-by: “Anton <a.kaymakchi@dodobrands.io>
Co-authored-by: Hiroshi Muraoka <h.muraoka714@gmail.com>
Co-authored-by: Aleksandr Kozlov <avlkozlov@avito.ru>
Co-authored-by: Evgeniy Kulikov <im@kulikov.im>
Co-authored-by: Takayuki WATANABE <takanabe.w@gmail.com>
Co-authored-by: Chris Lee <chris@delightroom.com>
Co-authored-by: Myroslav Gavryliak <efristical@gmail.com>
Co-authored-by: Daniel Metz <danielmmetz@gmail.com>
Co-authored-by: Justin Clift <jclift@dgitsystems.com>
Co-authored-by: Justin Judd <Justin@justinjudd.org>
Co-authored-by: sivchari <shibuuuu5@gmail.com>
Co-authored-by: Henry Foster <ahoy>
Co-authored-by: thorntonmc <mcthornton5@gmail.com>
Co-authored-by: Yoshio HANAWA <y@hnw.jp>
Co-authored-by: Valerian Saliou <valerian@valeriansaliou.name>
Co-authored-by: Pavel Lakosnikov <plakosnikov@avito.ru>
Co-authored-by: norabal <norabal.works@gmail.com>
Co-authored-by: Peter Kristensen <peter@ptx.dk>
Co-authored-by: Ryota <rytswd@gmail.com>
Co-authored-by: Karl-Johan Grahn <6355577+karl-johan-grahn@users.noreply.github.com>
Co-authored-by: Karl-Johan Grahn <Karl.Johan.Grahn@sinch.com>
Co-authored-by: Karl Keefer <karl@karlkeefer.com>
Co-authored-by: Rafael Almeida <rafaelcpalmeida@users.noreply.github.com>
Co-authored-by: James Loh <git@jloh.co>
Co-authored-by: Alexandre Bourget <alex@bourget.cc>
Co-authored-by: Chris Toshok <toshok@hound.sh>
Co-authored-by: Itay Donanhirsh <itay@bazoo.org>
Co-authored-by: amelia gapin <amelia@entirelyamelia.com>
Co-authored-by: Steffen Mahler <steffen.mahler@haendlerbund.de>
Co-authored-by: hidenami-i <shuhei.iwamoto.work@gmail.com>
Co-authored-by: Koichi Shiraishi <zchee.io@gmail.com>
Co-authored-by: Matthias Dötsch <matze@mdoetsch.de>
Co-authored-by: Leo Zhang <leo@leozhang.me>
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.

None yet

3 participants