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
Make compaction available for Oj::Doc
#650
Conversation
Formatting done using VSCode's C extension and using tabular indent. https://lea.verou.me/2012/01/why-tabs-are-clearly-superior/ Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
Introduce compaction to `fast.c`, only if `rb_gc_mark_movable` is available. Otherwise, keep a version that supports ruby <2.7. Since `rb_data_object_wrap` and `TypedData_Wrap_Struct` is available in every supported versions (eg >=2.4.10), I've also removed support for earlier versions. Result for compaction is great. I've tested it by allocating and removing arbitrary sized json. For a count of 70 pages, we can see that we used to need 68 pages after compaction,and now 61 pages. Of course this test include also lots of noise, but if we count pinned objects (objects that are marked in C extension as uncollectible), we can see that for a benchmark with no allocations, we have the same amount as a benchmark with this commit with thousands of allocations (499 pinned objects total), and before this, `Oj::Doc` used to generate 5k more objects (5499 pinned objects total). Hence this is a win, see PR for a graphical result. Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
The benchmark results are good but I'm not happy with the code reformatting. The fast.c code is now formatted much different than the rest of the code. Please don't take that as an incentive to reformat the whole project. I prefer the format I use. I like having the code line up nicely which it is with 4 space tabs. I also prefer the open The compaction is a nice addition. |
@ohler55 may I still format while using both 4 spaces and Or on github, where we can see that there is no identation inside functions: Lines 293 to 310 in a4f4590
|
Here's an option that seems to fit your coding style: # .clang-format
BasedOnStyle: Mozilla
IndentWidth: 4
BreakBeforeBraces: Attach
ColumnLimit: 0
AlwaysBreakAfterReturnType: AllDefinitions
PointerBindsToType: false (See https://releases.llvm.org/11.0.0/tools/clang/docs/ClangFormatStyleOptions.html for options) EDIT: you can see it with the latest commit 😄 The latest edit is as close as it can be as what you seem to expect: doesn't change anything but tabs into spaces. If this is now okay for you, I can squash before merging, eventually apply formatting changes to the whole stack and dig for space sensitive macros if there are any. And tell me if you want I to add compaction support for rails.c and valstack.c (maybe in a follow-up PR)! |
47d7f12
to
36f21d9
Compare
Better. So you are using a code formatting tool. Is there a way to get struct member and variable declaration to line up? For example, instead of: typedef struct Foo {
int a;
char *b;
} Something like this: typedef struct Foo {
int a;
char *b;
} |
@ohler55 there is a way with typedef struct _doc {
Leaf data;
Leaf * where; // points to current location
Leaf where_path[MAX_STACK]; // points to head of path
char * json;
unsigned long size; // number of leaves/branches in the doc
VALUE self;
Batch batches;
struct _batch batch0;
} * Doc; And yeah, I'm using clang's tool which should be pretty portable ! It seems that the alignement you desire is not implemented because of some situations where it cannot apply: https://stackoverflow.com/a/40312667/6320039 |
Thats too bad. Not sure which I prefer of the two choices. Maybe the use of AlignConsecutiveDeclaration and then I'll use a quick emacs macro to fix it up. It might be time to set up a nice formatter though. Something like what the golang goimports does. Super nice with go code anyway. |
@ohler55 unfortunately the implementation of that precise feature seems to have been waiting for a long time (https://reviews.llvm.org/D27651). I've looker a bit at possible alternative, the only one viable is https://github.com/uncrustify/uncrustify. However, its settings are very hard to understand. I've spent way too much time on that option.. If you want to take a look yourself, here are the two important entry points IMHO:
The other issue with uncrustify is that it is also less used, and harder for newcomer to contribute to Oj. I would use clang-format and make a concession on either variable or star alignment. (my favorite is star alignment, I've seen that variable alignement messes way too much with git's history, making it harder to blame or just navigate through) Let me know what you decide ! |
Okay, let's go with no alignment. Disappointing but maybe there is something out there that does the job. I'll look a little bit too. You do make a good point about the clang formatter vs uncrustify. Let me know when you are ready and I'll approve and merge. |
Using a `.clang-format` file, we fixed formatting rules to make sur they fit as much as possible current existing code, while still fixing spaces and tabs mixing issues. Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
ef1516d
to
11cc133
Compare
@ohler55 I've updated my commits. Since you mentionned having the same formatting for all files, I've ran clang format for every file in the last commit. Feel free to discard that one (or ask me to) if you are not OK for such a huge change right now. I would definitely understand seeing the 10k diff.. |
I merged and then reverted. I took your suggestion and set up a .clang-format file but for some reason when starting with the conversions you made the formatter refused to honor the setting I used. I'm trying to resolve that now. I certainly want the changes you made for the compaction but they are kind of lost in the complete reformatting of all the files. |
The only way I could get the changes to stick was by starting with the original. It seems some of the formatting considers previous formatting. That or I don't know how to configure the clang-format tool correctly. Anyway the code has been reformatted using the checked in clang-format configuration file in the repo root. It does not contain your changes for compaction. If you can make those changes on the current dev without reformatting all the files that would be great. If not I'll do a visual diff or look at the earlier commits and put in the changes. I'd rather give you credit for the compaction changes though if you don't mind the extra effort. |
Introduce compaction to `fast.c`, only if `rb_gc_mark_movable` is available. Otherwise, keep a version that supports ruby <2.7. Since `rb_data_object_wrap` and `TypedData_Wrap_Struct` is available in every supported versions (eg >=2.4.10), I've also removed support for earlier versions. Result for compaction is great. I've tested it by allocating and removing arbitrary sized json. For a count of 70 pages, we can see that we used to need 68 pages after compaction,and now 61 pages. Of course this test include also lots of noise, but if we count pinned objects (objects that are marked in C extension as uncollectible), we can see that for a benchmark with no allocations, we have the same amount as a benchmark with this commit with thousands of allocations (499 pinned objects total), and before this, `Oj::Doc` used to generate 5k more objects (5499 pinned objects total). Hence this is a win, see ohler55#650 for a graphical result. Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
* Make compaction available for `Oj::Doc` Introduce compaction to `fast.c`, only if `rb_gc_mark_movable` is available. Otherwise, keep a version that supports ruby <2.7. Since `rb_data_object_wrap` and `TypedData_Wrap_Struct` is available in every supported versions (eg >=2.4.10), I've also removed support for earlier versions. Result for compaction is great. I've tested it by allocating and removing arbitrary sized json. For a count of 70 pages, we can see that we used to need 68 pages after compaction,and now 61 pages. Of course this test include also lots of noise, but if we count pinned objects (objects that are marked in C extension as uncollectible), we can see that for a benchmark with no allocations, we have the same amount as a benchmark with this commit with thousands of allocations (499 pinned objects total), and before this, `Oj::Doc` used to generate 5k more objects (5499 pinned objects total). Hence this is a win, see #650 for a graphical result. Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com> * Change `clang-format` -> `.clang-format` According to clang's documentation: > When using `-style=file`, **clang-format** for each input file will > try to find the `.clang-format` file located in the closest parent > directory of the input file. When the standard input is used, the > search is started from the current directory. Hence the `.clang-format` seems more appropriate Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
Note for review: I've made two commits, the first one is just a code formatting since it use to mix space and tabs and was really hard to read. The second one contains my work.
Also, I can consider doing this work for
rails.c
andvalstack.c
, but the patch is big enough for you to review in the first time! I'd be glad to do so later :)Introduce compaction to
fast.c
, only ifrb_gc_mark_movable
isavailable. Otherwise, keep a version that supports ruby <2.7.
Since
rb_data_object_wrap
andTypedData_Wrap_Struct
is availablein every supported versions (eg >=2.4.10), I've also removed support
for earlier versions.
Result for compaction is great. I've tested it by allocating and
removing arbitrary sized json. For a count of 70 pages, we can see
that we used to need 68 pages after compaction,and now 61 pages. Of
course this test include also lots of noise, but if we count pinned
objects (objects that are marked in C extension as uncollectible),
we can see that for a benchmark with no allocations, we have the
same amount as a benchmark with this commit with thousands of allocations
(499 pinned objects total), and before this,
Oj::Doc
used to generate5k more objects (5499 pinned objects total).
Hence this is a win.
Here's the result of my tests after a
GC.compact
(reddots are pinned objects):And here are the scripts:
Details
This one was given by tenderlove: https://gist.github.com/tenderlove/578cfb4ce677465d0a8ceb4362928a89