-
Notifications
You must be signed in to change notification settings - Fork 22
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
BAAS-28253: Add optional tick tracking at the function level #115
Changes from 16 commits
b14fe92
5a636fa
ebff8e5
d9a1535
aae390a
cb34f32
5f52315
6dcfbe8
3a14fee
7e6ac5f
a4e34da
608e641
6f9d567
c47f607
f02d9b0
0ff8462
c0ed032
4fe0248
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -204,6 +204,9 @@ | |
limiter *rate.Limiter | ||
limiterTicksLeft int | ||
ticks uint64 | ||
|
||
tickMetricTrackingEnabled bool | ||
tickMetrics map[string]uint64 | ||
} | ||
|
||
func (self *Runtime) Ticks() uint64 { | ||
|
@@ -489,7 +492,9 @@ | |
} | ||
r.vm.init() | ||
|
||
r.tickMetrics = make(map[string]uint64) | ||
|
||
r.SetRateLimiter(rate.NewLimiter(rate.Inf, maxInt)) | ||
} | ||
|
||
func (r *Runtime) typeErrorResult(throw bool, args ...interface{}) { | ||
|
@@ -1528,6 +1533,7 @@ | |
vm.clearStack() | ||
} else { | ||
vm.stack = nil | ||
vm.profileTicks() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this we would miss the ticks tracked for the above vm.runTry
It could turn out that this isn't necessary because it is captured by the pushCtx/restoreCtx, but I don't think it hurts to have this in place just to make sure we capturing any lingering ticks for the program. Worst case scenario is there are no new ticks and we add 0 for the program. If we think this clutters the code I can dig more into this and see if it is necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping it works for me. |
||
vm.prg = nil | ||
vm.setFuncName("") | ||
r.leave() | ||
|
@@ -3229,3 +3235,17 @@ | |
} | ||
return defProto | ||
} | ||
|
||
func (self *Runtime) EnableTickMetricTracking() { | ||
self.tickMetricTrackingEnabled = true | ||
} | ||
|
||
func (self *Runtime) DisableTickMetricTracking() { | ||
self.tickMetricTrackingEnabled = false | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we add a comment for these exported functions? |
||
|
||
// TickMetrics returns a map of ticks used per function. | ||
// This function is not thread-safe and should only be called at the end of the function execution. | ||
func (self *Runtime) TickMetrics() map[string]uint64 { | ||
return self.tickMetrics | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -594,3 +594,130 @@ func TestStashMemUsage(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
func TestTickTracking(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] The one test I'm missing here is a function that throws an error. I was playing around with popCtx vs restoreCtx and most of the times profiling from either yields the same results. The only exception was when I was using it inside of the restoreCtx (like you are in this PR) where I noticed different ticks values when my function was throwing an error. Just thought it would be useful to have that case here. Realistically it will pass the test anyway and we won't see much different but profiling from popCtx only was missing out on some ticks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! I added a test to cover that we track ticks for a function that errors. 👍 |
||
tests := []struct { | ||
name string | ||
script string | ||
tickMetricTrackingEnabled bool | ||
expectedTickMetricsKeys []string | ||
}{ | ||
{ | ||
name: "should track looping function ticks when tick tracking is enabled", | ||
script: ` | ||
function f() { | ||
for (var i = 0; i < 100; i++) { | ||
} | ||
} | ||
f() | ||
`, | ||
tickMetricTrackingEnabled: true, | ||
expectedTickMetricsKeys: []string{"test.js.", "test.js.f"}, | ||
}, | ||
{ | ||
name: "should track larger looping function ticks when tick tracking is enabled", | ||
script: ` | ||
function f() { | ||
for (var i = 0; i < 1000; i++) { | ||
} | ||
} | ||
f() | ||
`, | ||
tickMetricTrackingEnabled: true, | ||
expectedTickMetricsKeys: []string{"test.js.", "test.js.f"}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [opt] Continuing this thread here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooo I really like |
||
}, | ||
{ | ||
name: "should track fib function ticks when tick tracking is enabled", | ||
script: ` | ||
function fib(n) { | ||
if (n < 2) return n; | ||
return fib(n - 2) + fib(n - 1); | ||
} | ||
fib(35); | ||
`, | ||
tickMetricTrackingEnabled: true, | ||
expectedTickMetricsKeys: []string{"test.js.", "test.js.fib"}, | ||
}, | ||
{ | ||
name: "should not track function ticks when tick tracking is disabled", | ||
script: ` | ||
function f() { | ||
for (var i = 0; i < 100; i++) { | ||
} | ||
} | ||
f() | ||
`, | ||
tickMetricTrackingEnabled: false, | ||
expectedTickMetricsKeys: []string{}, | ||
}, | ||
{ | ||
name: "should track ticks from a class method", | ||
script: ` | ||
class Car { | ||
constructor(brand) { | ||
this.brand = brand; | ||
} | ||
|
||
drive() {} | ||
honk() {} | ||
} | ||
const car = new Car("Tesla") | ||
car.drive() | ||
car.honk() | ||
`, | ||
tickMetricTrackingEnabled: true, | ||
expectedTickMetricsKeys: []string{"test.js.", "test.js.drive", "test.js.honk"}, | ||
}, | ||
{ | ||
name: "should track ticks from nested functions", | ||
script: ` | ||
function outerFunction() { | ||
function firstNestedFunction() { | ||
function secondNestedFunction() { | ||
function thirdNestedFunction() {} | ||
thirdNestedFunction(); | ||
} | ||
secondNestedFunction(); | ||
} | ||
firstNestedFunction(); | ||
} | ||
outerFunction(); | ||
`, | ||
tickMetricTrackingEnabled: true, | ||
expectedTickMetricsKeys: []string{ | ||
"test.js.", | ||
"test.js.outerFunction", | ||
"test.js.firstNestedFunction", | ||
"test.js.secondNestedFunction", | ||
"test.js.thirdNestedFunction", | ||
}, | ||
}, | ||
} | ||
|
||
for _, tc := range tests { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [q1]can we add some more tests such as executing executing class functions and multiple nested functions? [q2] Also, do you mind sharing the metrics that we would generate from baas when using a dependency such as the aws sdk? The easiest way to do this might be to run our dependencies tests in evergreen with the tracking enabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah absolutely!
Would that data be in splunk? Or would we need to parse the output of the tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Evergreen task with tick metric tracking hardcoded to true. Will update this thread with the results. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. I'm curious to see if the current approach makes it easy enough for us to identify which function is consuming the most amount of this. Let me know when this is ready. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm for some reason the logs aren't showing the "tick metrics" logs for that evergreen run. I think my local testing might alleviate some of your concerns though.
The majority of usage for this Buffer.from example is BufferJS::utf8ToBytes and BufferJS::blitBuffer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll also callout that these tick metrics are flat (not cumulative) in that we are measuring the specific time only for that function. So in this case we would know that we would want to have a native implementation in Go for utf8ToBytes and probably blitBuffer as well, which is nice because this allows us to not have to potentially waste time porting over the entire Buffer.from implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I have a feeling this is happening because of how tick metrics are aggregated and published asynchronously. It doesn't even look like the cmd/server/main.go executes which is where this asynchronous logic would start. I went ahead and set this up manually though, here are the results when running against aws-sdk-v3/s3 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmac lol There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh nice work, hello mr Buffer not surprised to see you up there! |
||
t.Run(tc.name, func(t *testing.T) { | ||
vm := New() | ||
if tc.tickMetricTrackingEnabled { | ||
vm.EnableTickMetricTracking() | ||
} | ||
|
||
prg := MustCompile("test.js", tc.script, false) | ||
_, err := vm.RunProgram(prg) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
actualTickMetrics := vm.TickMetrics() | ||
|
||
if len(actualTickMetrics) != len(tc.expectedTickMetricsKeys) { | ||
t.Fatalf("Unexpected tickMetrics length: Actual: %v Expected length: %v", actualTickMetrics, len(tc.expectedTickMetricsKeys)) | ||
} | ||
|
||
for _, key := range tc.expectedTickMetricsKeys { | ||
if actualTickMetrics[key] <= 0 { | ||
t.Fatalf("Unexpected tickMetrics for key: %v. Expected a positive tick value but received: %v", key, actualTickMetrics[key]) | ||
} | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to make this map get initialized per the functionTickTrackingEnabled setting but this actual would cause a problem (as demonstrated by the test). We need to have the vm object in order to enable the tick tracking but we also check if it is enabled when we create the vm object. So I think its better to just always initialize this map.
(^ snippet from the unit test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[opt] I would document this in code