Optimize log print by using html format #479

woehrl01 · 2017-03-13T18:31:12Z

See #453. Optimizes the node log print by generating some enum text via enum.py and moving printing to new functions to reduce boilerplate code.

Changes the log output to format the nodes in html to be able to copy paste it into browsers for quick debugging.

Hides all default values.

emilsjolander

Good job! Lets some initial comments

emilsjolander · 2017-03-14T11:04:22Z

yoga/YGEnumsPrint.h

+
+YG_EXTERN_C_BEGIN
+
+void YGLogAlign(YGLogLevel logLevel, const char * param, YGAlign value){


What is param ? Could you choose a more descriptive name?

Also could you mark all parameters as const?

File is names YGEnumsPrint but functions are named YGLogX. The mix of Print and Log is a bit odd.

emilsjolander · 2017-03-14T11:09:04Z

yoga/YGEnumsPrint.h

+      break;
+  }
+  if(param == NULL) {
+    YGLog(logLevel, text);


I would rather the functions in this file not return log but instead return a statically allocated string which is logged by the caller.

I also thought of this, but this makes the callside a bit more boilerplate, not sure if we want this.

Could you give it a try? I think it might be worth it. You can always abstract if if(param == null) part into another function.

emilsjolander · 2017-03-14T11:13:23Z

enums.py

@@ -146,6 +156,34 @@ def to_java_upper(symbol):
        f.write('\n')
    f.write('YG_EXTERN_C_END\n')

+# write out C headers for printing
+with open(root + '/yoga/YGEnumsPrint.h', 'w') as f:


Could this just be part of YGEnums.h? Collocating this stuff is usually pretty nice

emilsjolander · 2017-03-14T11:16:19Z

yoga/Yoga.c

+    YG_LOG_PRINT_NONZERO_EDGE(type, End);                                                    \
+  \
+}
+
 static bool YGFourValuesEqual(const YGValue four[4]) {


This function doesn't even handle start & end or vertical & horizontal so would love it fixes for that were included here as well 👍

emilsjolander · 2017-03-14T11:17:29Z

yoga/Yoga.c

+YGPrintNumberIfNotZero(YG_STRING(type) YG_STRING(edge), \
+                       YGComputedEdgeValue(node->style.type, YGEdge##edge, &YGValueZero));
+
+#define YG_LOG_PRINT_NONZERO_EDGES(type)                                                     \


This could probably be a function instead of a macro which takes a array of YGValues

this only works nicely if we write the position in lowercase, example: margin-left instead of current marginLeft. Could be nicer if we write html instead (see suggestion below)

Not sure I fully understand

If we loop over the array we could use YGLogEdge to stringify YGEdge but that returns only the edge in lowercase. So we need some fiddeling to uppercase if we want to stick to something like marginLeft.

But as you think that an html layout would be a great improvment, this is more easily doable (no need to uppercase). I'll update the PR with your suggestions and we can discuss on top of that, what do you think?

sounds good!

woehrl01 · 2017-03-14T11:39:17Z

Thanks for the comments, I'll try to work these in.

Additonally I'm thinking if it would be more useful if we output those values as html instead of JSONlike, to be able do copy paste possible wrong layouts more easily.

But I don't know how the output is used currently (e.g. ReactNative).

emilsjolander · 2017-03-14T12:15:44Z

Outputting a html layout would be amazing. This isn't really used a lot so I think outputting html will make it much more useful. Love the idea.

woehrl01 · 2017-03-14T22:10:54Z

I added html output and removed the usage for a macro, there are some small things which aren't that nice yet (e.g. "" usage). Those will be removed later, when I find a better way for this.

This function doesn't even vertical & horizontal ...

I can't add those as accessing this values lead to an YG_ASSERT error.

Could this just be part of YGEnums.h? Collocating this stuff is usually pretty nice

not really as we get some linker errors later, we would need to add the function templates to the header and provide an additional *.c file. Should we go this way?

File is names YGEnumsPrint but functions are named YGLogX

I change this if we want to keep it that way, depends on the previous.

woehrl01 · 2017-03-15T07:53:52Z

I'm not sure if we should print pt, if we want to use the output as input for gentest, we should print px, to not get different outputs based on those different scale factors. What do you think?

emilsjolander · 2017-03-15T11:46:58Z

we would need to add the function templates to the header and provide an additional *.c file. Should we go this way?

Yeah i'm totally ok with a YGEnums.c file.

I'm not sure if we should print pt, if we want to use the output as input for gentest, we should print px, to not get different outputs based on those different scale factors. What do you think?

perhaps use px if scale factor is 1 otherwise pt?

I can't add those as accessing this values lead to an YG_ASSERT error.

Actually we probably should not be calling YGComputedEdgeValue() here are we want to print the value which was set on the style, not the computed value.

woehrl01 · 2017-03-15T12:52:02Z

perhaps use px if scale factor is 1 otherwise pt?

isn't css using different factors for pt and px? I'd prefere sticking with just px.

print the value which was set on the style, not the computed value.

correct, I'll change it accordingly.

emilsjolander · 2017-03-15T17:47:13Z

isn't css using different factors for pt and px? I'd prefere sticking with just px.

Ah yeah that sounds good.

woehrl01 · 2017-03-16T08:04:04Z

@emilsjolander any idea why the Travis build fails for c?

emilsjolander · 2017-03-16T11:28:37Z

@woehrl01 not sure. Also not sure why there is no output in the logs. Can you run the tests locally?

emilsjolander

Can you post a sample of the output as well? 👍

emilsjolander · 2017-03-16T11:29:48Z

tests/YGLoggerTest.cpp

+#include <yoga/Yoga.h>
+
+static char writeBuffer[4096];
+static int _unmanagedLogger(YGLogLevel level, const char *format, va_list args) {


va_list is defined in stdarg.h right?

should I include this header?

emilsjolander · 2017-03-16T11:30:19Z

yoga/YGEnums.c

+
+#include "YGEnums.h"
+
+const char * YGStringifyAlign(const YGAlign value){


YGAlignToString

Also char *YGStringifyAlign instead of char * YGStringifyAlign (asterix placement)

emilsjolander · 2017-03-16T11:31:44Z

yoga/YGEnums.c

+      return "space-between";
+    case YGAlignSpaceAround:
+      return "space-around";
+    default:


Is the default needed? I think this switch should be exhaustive. Not supplying default will allow the compiler to warn when not all cases are specified.

not exactly as a default. but at least as a "fall-through". Will remove the default and put the return "unknown" after the switch

What is the benefit of this?

the compiler needs it, the function doesn't have a return statement otherwise for all call cases. (e.g. calling it an invalid argument)

emilsjolander · 2017-03-16T11:32:07Z

yoga/YGEnums.h

@@ -132,4 +132,20 @@ typedef YG_ENUM_BEGIN(YGWrap) {
  YGWrapWrapReverse,
 } YG_ENUM_END(YGWrap);

+
+WIN_EXPORT const char * YGStringifyAlign(const YGAlign value);


char *YG...

Can you put each toString function bellow the corresponding enum declaration?

woehrl01 · 2017-03-16T11:54:28Z

Current output looks like:

<div layout="width: 0; height: 0; top: 0; left: 0;" style="flex-direction: column; justify-content: flex-start; align-items: stretch; align-content: flex-start; flex-grow: 0; flex-shrink: 0; " >
  <div layout="width: 0; height: 0; top: 0; left: 0;" style="flex-direction: column; justify-content: flex-start; align-items: stretch; align-content: flex-start; flex-grow: 0; flex-shrink: 0; " ></div>
  <div layout="width: 0; height: 0; top: 0; left: 0;" style="flex-direction: column; justify-content: flex-start; align-items: stretch; align-content: flex-start; flex-grow: 0; flex-shrink: 0; " ></div>
</div>

emilsjolander · 2017-03-16T11:57:04Z

Current output looks like:

Lovely!

woehrl01 · 2017-03-16T12:00:17Z

should I hide all default values e.g. for flex-direction, justify-content, too?

What about flex ? and it's special case negative flex?

emilsjolander · 2017-03-16T12:04:55Z

should I hide all default values

Good point. I think hiding defaults is good.

What about flex

Let's print it if it is set. negative or not.

woehrl01 · 2017-03-18T08:51:21Z

Found the root cause of the failing travis test. All DEATH tests should be named like *DeathTest so they are executed at first. As they are run later, they output seems to be overridden by our new logger tests. See: https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md

use correct edge remove sln added unittest for logger, reduced YGEnumsPrint.h size free node added additonal test with percentage and margin format and dedup renamed macros macro prefix, put auto in quotes added log output for flexWrap and display use power of 8 for array size fix clang macro expansion no macros, html output const added info for custom measure function typo compare to default values do not print auto, remove need for "" rename headers and move to .c file fix .c generation do not print align-self if default format use safer snprintf instead of strcat use fixed buf do not print default values, rename Stringify to ToString remove tmp add ygenum.c to vcxproj try to make travis happy be temporarily more verbose to see why travis fails mark death tests according to gtests suggestions setting NULL on logger should return to DefaultLog disable verbosity fix c# unittest c# test c# test c# test

arcanis · 2017-03-25T13:10:29Z

The JSON format had nice properties that could have been used for features (for example, extracting a tree from an application and recreating it standalone via the JS bindings in order to better debug the library). Is there plans to support multiple logging outputs?

woehrl01 · 2017-03-25T14:58:47Z

Multiple logging outputs could be doable. Did you use the JSONlike output? As the output wasn't exactly JSON ' instead of " for properties. Even so it could be nice to use the output directly for javascript. I think the HTML output is more generic as you could use gentest.py to generate your DOM easily for any supported language.

emilsjolander · 2017-03-30T09:24:51Z

@woehrl01 I agree we should stick with html output for now.
@arcanis I'm definetly not against multiple output formats but for now html is more useful

facebook-github-bot · 2017-03-30T09:32:11Z

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: See facebook/yoga#453. Optimizes the node log print by generating some enum text via ```enum.py``` and moving printing to new functions to reduce boilerplate code. Changes the log output to format the nodes in html to be able to copy paste it into browsers for quick debugging. Hides all default values. Closes facebook/yoga#479 Reviewed By: gkassabli Differential Revision: D4802184 Pulled By: emilsjolander fbshipit-source-id: 143bd63cbc31fb0755d711062cb4e6a448049ba3

facebook-github-bot added the CLA Signed label Mar 13, 2017

woehrl01 changed the title ~~WIP: Optimize debug print~~ WIP: Optimize log print Mar 14, 2017

emilsjolander reviewed Mar 14, 2017

View reviewed changes

emilsjolander suggested changes Mar 16, 2017

View reviewed changes

woehrl01 changed the title ~~WIP: Optimize log print~~ Optimize log print by using html format Mar 16, 2017

woehrl01 force-pushed the print-debug branch from 8ec1a7f to 1fd6206 Compare March 21, 2017 21:38

facebook-github-bot closed this in 586b570 Apr 3, 2017


		YG_EXTERN_C_BEGIN

		void YGLogAlign(YGLogLevel logLevel, const char * param, YGAlign value){


		#include "YGEnums.h"

		const char * YGStringifyAlign(const YGAlign value){

Optimize log print by using html format #479

Optimize log print by using html format #479

Conversation

woehrl01 commented Mar 13, 2017 • edited Loading

emilsjolander left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woehrl01 commented Mar 14, 2017

emilsjolander commented Mar 14, 2017

woehrl01 commented Mar 14, 2017

woehrl01 commented Mar 15, 2017 • edited Loading

emilsjolander commented Mar 15, 2017

woehrl01 commented Mar 15, 2017

emilsjolander commented Mar 15, 2017

woehrl01 commented Mar 16, 2017

emilsjolander commented Mar 16, 2017

emilsjolander left a 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

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

woehrl01 Mar 16, 2017 • 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

woehrl01 commented Mar 16, 2017

emilsjolander commented Mar 16, 2017

woehrl01 commented Mar 16, 2017 • edited Loading

emilsjolander commented Mar 16, 2017

woehrl01 commented Mar 18, 2017 • edited Loading

arcanis commented Mar 25, 2017 • edited Loading

woehrl01 commented Mar 25, 2017

emilsjolander commented Mar 30, 2017

facebook-github-bot commented Mar 30, 2017

woehrl01 commented Mar 13, 2017 •

edited

Loading

woehrl01 commented Mar 15, 2017 •

edited

Loading

woehrl01 Mar 16, 2017 •

edited

Loading

woehrl01 commented Mar 16, 2017 •

edited

Loading

woehrl01 commented Mar 18, 2017 •

edited

Loading

arcanis commented Mar 25, 2017 •

edited

Loading