Avoid using StringTable for runtime data. by ozevren · Pull Request #1974 · istio/istio

ozevren · 2017-12-04T21:45:24Z

What this PR does / why we need it:
This fixes an issue in the evaluator that causes runtime strings to be captured in the strings table. With this fix:

The strings are stored in heap, similar to other interface{} objects.
The comparisons are performed using regular string comparison.
The size-based expression cache eviction algorithm (& its parameter) is removed.
Renamed the method on StringTable from GetID => Add for clarity.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
fixes #1369
Special notes for your reviewer:
This will most likely conflict with the pkg/server change. I'll wait until that one lands, before checking this in.

Release note:

geeknoid

/lgtm
/approve

geeknoid · 2017-12-05T04:01:30Z

mixer/pkg/il/evaluator/evaluator.go

I really don't like "cacheSize" as a name since it's ambiguous between "size in bytes" and "size in entries". maxCacheEntries is usually a better choice for this kind of name IMO.

Noted. Hopefully, this will not survive very long, if I can land the CompiledExpression work soon.

istio-merge-robot · 2017-12-05T23:21:23Z

@ozevren PR needs rebase

…uation. This fixes the tainting of the StringMap problem.

This ensures that the method would not be called from any execution level context.

evaluator code. This is not needed anymore with use of heap for storing strings during evaluation.

codecov · 2017-12-06T17:12:39Z

Codecov Report

Merging #1974 into master will increase coverage by 2.22%.
The diff coverage is 72.51%.

@@            Coverage Diff             @@
##           master    #1974      +/-   ##
==========================================
+ Coverage   79.81%   82.03%   +2.22%     
==========================================
  Files          75      202     +127     
  Lines        6890    16819    +9929     
==========================================
+ Hits         5499    13798    +8299     
- Misses       1104     2530    +1426     
- Partials      287      491     +204

Flag	Coverage Δ
#broker	`46.09% <ø> (-1.18%)`	⬇️
#mixer	`83.52% <72.51%> (?)`
#pilot	`80.81% <ø> (+0.09%)`	⬆️
#security	`92.03% <ø> (+3.2%)`	⬆️

Impacted Files	Coverage Δ
mixer/pkg/il/interpreter/interpreter.go	`100% <ø> (ø)`
mixer/cmd/server/cmd/server.go	`0% <0%> (ø)`
mixer/pkg/il/interpreter/extern.go	`96.8% <100%> (ø)`
mixer/pkg/il/strings.go	`100% <100%> (ø)`
mixer/pkg/il/program.go	`100% <100%> (ø)`
mixer/pkg/il/evaluator/evaluator.go	`84.88% <100%> (ø)`
mixer/pkg/il/text/read.go	`94.47% <100%> (ø)`
mixer/pkg/il/builder.go	`100% <100%> (ø)`
mixer/pkg/il/interpreter/interpreterRun.go	`95.47% <69.91%> (ø)`
pilot/adapter/config/memory/monitor.go	`81.25% <0%> (-9.38%)`	⬇️
... and 144 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 704d5cc...ca3aa5d. Read the comment docs.

istio-merge-robot · 2017-12-06T17:12:56Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @geeknoid @ozevren

ozevren · 2017-12-06T17:13:36Z

Had to rebase. Can I get another /lgtm ?

geeknoid · 2017-12-06T17:17:16Z

/lgtm
/approve

istio-merge-robot · 2017-12-06T17:17:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~mixer/OWNERS~~ [geeknoid]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

istio-merge-robot · 2017-12-06T17:49:58Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-12-06T20:39:48Z

/test all [submit-queue is verifying that this PR is safe to merge]

ozevren · 2017-12-06T21:12:34Z

/test istio-pilot-e2e

istio-merge-robot · 2017-12-06T21:32:26Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-12-06T21:57:14Z

Automatic merge from submit-queue.

istio-testing added the release-note-none label Dec 4, 2017

googlebot added the cla: yes label Dec 4, 2017

ozevren requested a review from geeknoid December 4, 2017 21:45

istio-merge-robot added the do-not-merge/post-submit label Dec 4, 2017

ozevren added the do-not-merge Block automatic merging of a PR. label Dec 4, 2017

istio-merge-robot added do-not-merge/post-submit PostSubmit Failed/Contact Oncall and removed do-not-merge/post-submit labels Dec 4, 2017

geeknoid reviewed Dec 5, 2017

View reviewed changes

istio-testing assigned geeknoid Dec 5, 2017

istio-testing added the lgtm label Dec 5, 2017

istio-merge-robot added approved and removed do-not-merge/post-submit PostSubmit Failed/Contact Oncall labels Dec 5, 2017

ozevren added do-not-merge Block automatic merging of a PR. and removed do-not-merge Block automatic merging of a PR. labels Dec 5, 2017

istio-merge-robot removed the PostSubmit Failed/Contact Oncall label Dec 5, 2017

istio-merge-robot added needs-rebase Indicates a PR needs to be rebased before being merged PostSubmit Failed/Contact Oncall labels Dec 5, 2017

ozben added 3 commits December 6, 2017 09:04

Use heap, instead of StringMap when allocating strings during IL eval…

6825455

…uation. This fixes the tainting of the StringMap problem.

Rename the GetID method of Strings to Add, for clarity.

83a9601

This ensures that the method would not be called from any execution level context.

Remove the string-size expansion based eviction algorithm from the

3ef522c

evaluator code. This is not needed anymore with use of heap for storing strings during evaluation.

Fix minor formatting issues.

ca3aa5d

istio-merge-robot removed lgtm needs-rebase Indicates a PR needs to be rebased before being merged labels Dec 6, 2017

istio-testing added the lgtm label Dec 6, 2017

istio-merge-robot removed the PostSubmit Failed/Contact Oncall label Dec 6, 2017

istio-merge-robot added PostSubmit Failed/Contact Oncall and removed PostSubmit Failed/Contact Oncall labels Dec 6, 2017

istio-merge-robot merged commit 4308f8f into istio:master Dec 6, 2017

ozevren deleted the string-table branch December 7, 2017 00:14

Conversation

ozevren commented Dec 4, 2017

Uh oh!

geeknoid left a comment

Choose a reason for hiding this comment

Uh oh!

geeknoid Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

ozevren Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Dec 5, 2017

Uh oh!

codecov bot commented Dec 6, 2017

Codecov Report

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

ozevren commented Dec 6, 2017

Uh oh!

geeknoid commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

ozevren commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants