Component benchmarks #65

ZoteTheMighty · 2018-04-12T17:48:12Z

Retrying my approach to tracking performance. This will gather stats about render calls and shouldUpdate calls, and should be able to provide info that would help indicate when to make a Component a PureComponent or vice versa, when to write a custom shouldUpdate, or when to try to cut down on the complexity of render methods.

I didn't find a particularly clean way to inject the instrumentation into the Component code without muddling it a bit, so I'd love suggestions for integrating it more gracefully (I left TODOs to this effect).

coveralls · 2018-04-12T17:50:09Z

Coverage increased (+0.7%) to 89.371% when pulling cd3e885 on ZoteTheMighty:component-benchmarks into bda5a4e on Roblox:master.

LPGhatguy

Should we instrument the reconciler as well so that we can capture functional or primitive components?

We still don't have a way to give a name to a functional component, but there's a tracking issue for it.

LPGhatguy · 2018-04-12T17:52:24Z

lib/Component.lua


 local Component = {}

+-- Locally cache tick so we can minimize impact of calling it for instrumentation
+local l_tick = tick


We can just call our local version tick, since Lua lets you do that.

AmaranthineCodices

This looks good, and should be really useful! I'm slightly concerned about printing tables to the output, given that:

The output window's size can vary - mine is fairly small, for example
The output window, by default, does not use a fixed-width font, so columns based on character counts aren't going to line up by default

Maybe it'd be useful to have an additional way to get the raw data being printed, so it can be visualized in a separate UI that can have a more robust table view?

EDIT: Oops! I missed your changes; disregard this review!

AmaranthineCodices · 2018-04-12T17:57:20Z

lib/Instrumentation.lua

+
+local Instrumentation = {}
+
+local ComponentStats = {}


nitpick: camelCase variable name

AmaranthineCodices · 2018-04-12T17:58:08Z

lib/Instrumentation.lua

+
+local function valuesByStat(t, f)
+	local a = {}
+	for _, obj in pairs(t) do table.insert(a, obj) end


Can we break this out of a single line?

AmaranthineCodices · 2018-04-12T17:59:09Z

lib/Instrumentation.lua

+		-- Percent of time shouldUpdate returns true
+		-- Percent of total shouldUpdate time by component
+
+local function valuesByStat(t, f)


Is there any particular reason this is returning an iterator instead of an array? Also, can we have more descriptive variable names in this function?

AmaranthineCodices · 2018-04-12T18:00:04Z

lib/Instrumentation.lua

+	local trackRenders = GlobalConfig.getValue("renderInstrumentation")
+
+	if not trackUpdates and not trackRenders then
+		print("No stats are being tracked!  Enable with Roact.setConfig({...})")


Roact.setGlobalConfig, not Roact.setConfig!

AmaranthineCodices · 2018-04-12T18:03:02Z

lib/Instrumentation.lua

+		end
+		print(statsRow)
+	end
+	if trackUpdates then


Are these stats terribly useful, given that they aren't attributed to specific components?

AmaranthineCodices · 2018-04-12T18:04:23Z

lib/init.lua

@@ -39,6 +40,7 @@ apply(Roact, {
 	PureComponent = PureComponent,
 	Event = Event,
 	Change = Change,
+	Instrumentation = Instrumentation,


We can probably only expose getCollectedStats and clearCollectedStats here instead of exposing the entire module.

AmaranthineCodices · 2018-04-12T18:05:09Z

lib/Instrumentation.lua

+	local i = 0      -- iterator variable
+	local iter = function ()   -- iterator function
+		i = i + 1
+		if a[i] == nil then


This entire if block can be simplified to return a[i].

AmaranthineCodices · 2018-04-12T18:05:25Z

lib/Instrumentation.lua

+	for _, obj in pairs(t) do table.insert(a, obj) end
+	table.sort(a, f)
+	local i = 0      -- iterator variable
+	local iter = function ()   -- iterator function


nitpick: no spaces between function and parentheses

AmaranthineCodices · 2018-04-12T18:09:30Z

lib/Instrumentation.lua

+	local entry = ComponentStats[name]
+	if not entry then
+		-- Use shortened name for convenience; 
+		local shortName = name:gsub("Connection","Conn")


Let's not shorten names like this - finding poorly-performing components could be tricky if the name gets squashed. If we absolutely must shorten names, we could try trimming the string after it passes a specific number of characters (maybe trim from the end, so ReallyLongName gets trimmed to something like ...ongName? Not sure about the exact semantics).

ZoteTheMighty · 2018-04-12T18:23:53Z

@AmaranthineCodices I actually removed printStats in the final commit for the reasons you mentioned. For now I don't have a solid way to output results that works for everyone's workflow. The goal would be to eventually have a Roblox Studio plugin that's capable of consuming and nicely displaying these stats.

Additionally, having a giant unwieldy print function floating around in the main codebase feels weird (partly why I scrapped my previous PR). The fact that Roblox Studio's default font for the Output window is not mono-spaced is a whole different can of worms...

Anyway, the printStats function should actually be excluded from the final PR. I can squash commits together if that'd make the history cleaner.

AmaranthineCodices · 2018-04-12T18:26:26Z

No, it's okay! I started reviewing before you pushed the second commit, and didn't see it before I submitted the review. Don't worry about squashing commits; @LPGhatguy will do that while merging. This looks good as of now, barring lpg's note about tick.

LPGhatguy · 2018-04-17T07:22:39Z

lib/init.lua

+apply(Roact, {
+	getCollectedStats = Instrumentation.getCollectedStats,
+	clearCollectedStats = Instrumentation.clearCollectedStats,
+})


We probably shouldn't put specific benchmarking functions on the top-level, at least not yet.

That's fair. I've removed it.

Would it be okay to expose them as a table, something like Roact.Instrumentation.getCollectedStats?

Heh, sounds closer to my original implementation. @LPGhatguy It would be good to expose them in some way so that there's some utility to these changes, especially for the sake of the other tooling I'm working on. Let me know what you think.

Putting them at Roact.Instrumentation.getCollectedStats would be fine.

The original version exposed them as Roact.getCollectedStats, that was the problematic variant!

The original version (a few commits ago) actually had the whole Instrumentation object exposed, but it makes more sense to expose only the needed methods and still do it indirectly.

ZoteTheMighty · 2018-04-19T03:07:15Z

@LPGhatguy Any other changes you'd like to see before merging this?

LPGhatguy · 2018-04-19T19:01:58Z

I'll pull this branch down and work with it locally a bit, and potentially have feedback!

LPGhatguy · 2018-04-20T00:34:32Z

lib/Config.lua

+	-- Enables instrumentation of shouldUpdate
+	["shouldUpdateInstrumentation"] = false,
+	-- Enables instrumentation of rendering
+	["renderInstrumentation"] = false,


Do you think it's valuable to toggle instrumentation like this individually?

When working with a profiling debugger, I imagine we would either want all of the information logged, or none of it!

I agree wholeheartedly, especially now that I'm faced with the idea of having my plugin behave accordingly if something is disabled. I'll make that change.

… methods directly in init.lua; add a bit of detail to some comments

…ly and made slight changes for clarity.

LPGhatguy · 2018-04-23T22:44:56Z

Okay, I'm moving forward with the merge now because this PR only minor changes to the actual reconciler code, and doesn't affect the public API (since it's marked UNSTABLE). That should give us flexibility to modify and improve the stats that we report going forward without worrying about developers depending on this API.

LPGhatguy reviewed Apr 12, 2018

View reviewed changes

AmaranthineCodices reviewed Apr 12, 2018

View reviewed changes

LPGhatguy reviewed Apr 17, 2018

View reviewed changes

ZoteTheMighty force-pushed the component-benchmarks branch 2 times, most recently from fbfc457 to 7d17213 Compare April 18, 2018 00:17

LPGhatguy reviewed Apr 20, 2018

View reviewed changes

ZoteTheMighty force-pushed the component-benchmarks branch from f032e73 to 77aaa43 Compare April 20, 2018 02:30

ZoteTheMighty added 8 commits April 23, 2018 14:41

Profile performance by directly instrumenting Component

d543d55

Minor cleanup, add some test cases

1d0d9ab

PR Comments: use 'tick' name for locally cached function, expose stat…

f57281d

… methods directly in init.lua; add a bit of detail to some comments

Remove exposure of instrumentation for now

4c1c6cc

Added that little extra bit of coverage to Component

e4090b7

Re-expose the instrumentation, but not from the top level

00a637c

Converged two separate config flags into one. Updated tests according…

ff5efb0

…ly and made slight changes for clarity.

Adjust public API to mark instrumentation API as unstable

cd3e885

ZoteTheMighty force-pushed the component-benchmarks branch from edd68a2 to cd3e885 Compare April 23, 2018 21:41

LPGhatguy merged commit cee83c6 into Roblox:master Apr 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Component benchmarks #65

Component benchmarks #65

ZoteTheMighty commented Apr 12, 2018

coveralls commented Apr 12, 2018 •

edited

Loading

LPGhatguy left a comment

LPGhatguy Apr 12, 2018

AmaranthineCodices left a comment •

edited

Loading

AmaranthineCodices Apr 12, 2018

AmaranthineCodices Apr 12, 2018

AmaranthineCodices Apr 12, 2018

AmaranthineCodices Apr 12, 2018

AmaranthineCodices Apr 12, 2018

AmaranthineCodices Apr 12, 2018 •

edited

Loading

AmaranthineCodices Apr 12, 2018

AmaranthineCodices Apr 12, 2018

AmaranthineCodices Apr 12, 2018

ZoteTheMighty commented Apr 12, 2018

AmaranthineCodices commented Apr 12, 2018

LPGhatguy Apr 17, 2018

ZoteTheMighty Apr 17, 2018 •

edited

Loading

AmaranthineCodices Apr 18, 2018

ZoteTheMighty Apr 18, 2018 •

edited

Loading

LPGhatguy Apr 18, 2018

ZoteTheMighty Apr 18, 2018 •

edited

Loading

ZoteTheMighty commented Apr 19, 2018

LPGhatguy commented Apr 19, 2018

LPGhatguy Apr 20, 2018

ZoteTheMighty Apr 20, 2018

LPGhatguy commented Apr 23, 2018

Component benchmarks #65

Component benchmarks #65

Conversation

ZoteTheMighty commented Apr 12, 2018

coveralls commented Apr 12, 2018 • edited Loading

LPGhatguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmaranthineCodices left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmaranthineCodices Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZoteTheMighty commented Apr 12, 2018

AmaranthineCodices commented Apr 12, 2018

Choose a reason for hiding this comment

ZoteTheMighty Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZoteTheMighty Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZoteTheMighty Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

ZoteTheMighty commented Apr 19, 2018

LPGhatguy commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LPGhatguy commented Apr 23, 2018

coveralls commented Apr 12, 2018 •

edited

Loading

AmaranthineCodices left a comment •

edited

Loading

AmaranthineCodices Apr 12, 2018 •

edited

Loading

ZoteTheMighty Apr 17, 2018 •

edited

Loading

ZoteTheMighty Apr 18, 2018 •

edited

Loading

ZoteTheMighty Apr 18, 2018 •

edited

Loading