Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Global debug flag, better error messages #188

Merged
merged 15 commits into from
Apr 19, 2019

Conversation

ZoteTheMighty
Copy link
Contributor

@ZoteTheMighty ZoteTheMighty commented Apr 3, 2019

Adds messages to all asserts. Also adds a convenience method for adjusting assert messages when asserts should only signify internal bugs.

Additionally, adds checks to a global debug flag called _G.ROACT_DEBUG, which will toggle whether or not we do asserts in our most performance sensitive code paths.

Repurposes Config to introduce flagged asserts. Uses mutation internally in Config so that consumers can grab the configuration at require time and refer to updated values after they get set.

So far, speed gains are fairly noticeable in most cases:

                             NewReconciler     NewReconcilerOpt  
  fancy (x2000)              1.8738s           1.7932s (-4.3%)   
  hello (x100000)            1.2871s           1.1330s (-12.0%)  
  lifecycle (x100000)        0.8759s           0.6985s (-20.2%)  
  nested-mount (x100000)     5.4379s           4.1647s (-23.4%)  
  nested-update (x100000)    4.3488s           3.5283s (-18.9%)  
  stateful-mount (x100000)   1.4395s           0.9646s (-33.0%)  
  stateful-update (x100000)  0.8171s           0.4630s (-43.3%)  
  update (x100000)           0.7654s           0.7499s (-2.0%)   

Checklist before submitting:

  • Added entry to CHANGELOG.md
  • Added/updated relevant tests
  • Added/updated documentation

@ZoteTheMighty
Copy link
Contributor Author

I dunno if this is the approach we want to align on, it seems... okay, but I wonder if it's just a little too messy/intrusive?

@ZoteTheMighty
Copy link
Contributor Author

Some thoughts on this:

  • First of all, I should probably remove/clean up GlobalConfig in this same commit. We'll probably make it no-op and issue a warning when interacted with.
  • There's one debug flag that controls all features: prop validate, argument type checks of both internal and external apis, and element tracing. It probably makes sense to break this up into separate buckets

@LPGhatguy
Copy link
Contributor

I opened rojo-rbx/rojo#160 for the tooling side implications of changes like this that we might be able to port and take advantage of.

@ZoteTheMighty ZoteTheMighty marked this pull request as ready for review April 19, 2019 21:24
Copy link
Contributor

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

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

LGTM

@ZoteTheMighty ZoteTheMighty merged commit 27ea43c into Roblox:new-reconciler Apr 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants