Skip to content
This repository was archived by the owner on Aug 30, 2019. It is now read-only.

obfuscate: clean up SQL obfuscator#487

Merged
gbbr merged 1 commit into
masterfrom
gbbr/cleanup-sql
Oct 10, 2018
Merged

obfuscate: clean up SQL obfuscator#487
gbbr merged 1 commit into
masterfrom
gbbr/cleanup-sql

Conversation

@gbbr

@gbbr gbbr commented Oct 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Renaming and simplifying comments
  • Removed agent.parse.error tag as it brought no real details about parsing errors.
  • Reset obfuscator only once, before it runs.

No apparent impact on performance:

name                      old time/op    new time/op    delta
Tokenizer/Escaping/512-8    7.27µs ± 0%    7.30µs ± 0%  +0.32%
Tokenizer/Grouping/199-8    4.16µs ± 0%    4.17µs ± 0%  +0.19%

name                      old alloc/op   new alloc/op   delta
Tokenizer/Escaping/512-8    4.40kB ± 0%    4.40kB ± 0%   0.00%
Tokenizer/Grouping/199-8    2.72kB ± 0%    2.72kB ± 0%   0.00%

name                      old allocs/op  new allocs/op  delta
Tokenizer/Escaping/512-8      87.0 ± 0%      87.0 ± 0%   0.00%
Tokenizer/Grouping/199-8      68.0 ± 0%      68.0 ± 0%   0.00%

@gbbr
gbbr requested a review from palazzem October 10, 2018 08:56
palazzem
palazzem reviewed Oct 10, 2018
View reviewed changes

@palazzem palazzem left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great cleanup!

Comment thread obfuscate/sql.go

func (t *sqlObfuscator) obfuscate(in string) (string, error) {
var out bytes.Buffer
t.reset(in)

@palazzem palazzem Oct 10, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it enough to reset it once? I guess it was redundant, but can you check the benchmarks (I think we should have something) to detect how many allocations / performance we have? Just for posterity, since this is a critical component that is called so many times.

@gbbr gbbr Oct 10, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have updated the PR description. There is a small difference in time, but I think that's negligible and most likely unrelated to the change. Happy to address performance improvements in another PR. How's it look?

@gbbr

gbbr commented Oct 10, 2018

Copy link
Copy Markdown
Contributor Author

@palazzem PTAL

@gbbr gbbr mentioned this pull request Oct 10, 2018
Closed
@gbbr
gbbr merged commit bda1ebb into master Oct 10, 2018
@gbbr
gbbr deleted the gbbr/cleanup-sql branch October 10, 2018 13:53
@gbbr gbbr added this to the 6.6.0 milestone Oct 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@palazzem palazzem palazzem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

6.6.0

Development

Successfully merging this pull request may close these issues.

2 participants

@gbbr @palazzem