Performance enhancement reducing clone #321

patefacio · 2018-08-22T20:54:44Z

No description provided.

- Renderer::render to not clone client provided Value - Added RefOrOwned to treat references and cloned data the same - Added CallStack which contains read-only borrow of user supplied context - Added StackFrame for macro calls and for loops - Added MacroCollection collect parsed `MacroDefinition`s by template

- `test_json_pointer` - `test_simple_for_loop` - `test_simple_nested_for_loop` - `test_simple_assignment` - `test_nested_for_var_access` - `test_error_render_field_unknown_in_forloop` to match error message

- Fixed copy/paste error: `current_key` now returns key, not value

- Added `CallStack::error_location` - Added `StackFrame::template_location` - Split `find_value` into `find_value_(in_frame_context|in_for_loop)` - Added back `set_global` support - Cleaned `render_node` call `chain_err`

- Updated `test_error_in_macro_location` error expectation - Updated `test_error_macros_self_inexisting` error expectation

- `add_set_values_in_context` working

- Makes `render_for` test pass

- Added back `do_super` - Added `template` and `template_root` to AstProcessor - Added `MAGICAL_DUMP_VAR` support - Added Include FrameType and `push_include_frame`

- Fixed lifetimes on `find_value_...` functions - Added `Accessor` impl for `CallStack` - Update `AstProcessor` to use process_path for var processing - Back to original benches, with one fix to `huge_loop` - Removed `square_brackets`, covered by `path_processor`

- Fixed `eval_macro_call` to lookup macro in macro collection - Renamed `from_template_root` -> `from_template_original`

- Removed unnecessary map creation of macro definition map - Added `macto_namespace` to StrackFrame for macro lookup - Clippy suggestions - Used original `get_error_location` - Added tests for path processing

Keats

Pretty huge diff, it's going to take me a while to review properly!

Left a couple of comments, the code could also use a rebase since there would be merge conflicts.

Keats · 2018-08-23T18:52:10Z

Cargo.toml

@@ -29,7 +29,10 @@ url = "1"
 humansize = "1"
 # used in date format filter
 chrono = "0.4"
+# used in instrument timing


the 3 dependencies below should be in dev-dependencies then?

Keats · 2018-08-23T18:53:06Z

src/bin/big_context_no_loop.rs

@@ -0,0 +1,104 @@
+extern crate tera;


is this file needed? can it be in bench instead?

Removed file.

Keats · 2018-08-23T18:53:57Z

src/renderer/ast_processor.rs

@@ -0,0 +1,1064 @@
+//! Responsible for walking ast and render implementation
+
+// --- module use statements ---


can you remove all those comments?

Keats · 2018-08-23T18:54:51Z

src/renderer/tests/errors.rs

+    // assert_eq!(
+    //     result.unwrap_err().iter().nth(1).unwrap().description(),
+    //     "Variable arr[a] can not be evaluated because: Variable `a` not found in context while rendering \'tpl\'"
+    // );


it should be uncommented no?

Replaced this with a test.

Keats · 2018-08-23T18:55:33Z

Cargo.toml

@@ -29,7 +29,10 @@ url = "1"
 humansize = "1"
 # used in date format filter
 chrono = "0.4"
+# used in instrument timing
+stopwatch = "^0.0.7"
+log = "^0.3.8"


was logging used only when debugging? can it be removed?

Removed stopwatch dependency. I do use log! by creating a macro called out! that points to log! for dev but can switch to println! if desire is to see logs from testing, since testing does not support.

I can remove this logging if you'd like, but maybe best to leave until you have time to review, in case need to review code more generally and logging might help. Whichever you prefer.

I believe I effectively did a rebase. My first and my first pull request, so I may have screwed it up. I hope it helps you diff more easily - but I'm not seeing how it helps so maybe I missed a step.

- Renderer::render to not clone client provided Value - Added RefOrOwned to treat references and cloned data the same - Added CallStack which contains read-only borrow of user supplied context - Added StackFrame for macro calls and for loops - Added MacroCollection collect parsed `MacroDefinition`s by template Added more success test cases - `test_json_pointer` - `test_simple_for_loop` - `test_simple_nested_for_loop` - `test_simple_assignment` - `test_nested_for_var_access` - `test_error_render_field_unknown_in_forloop` to match error message Added `test_simple_key_val_for_loop` - Fixed copy/paste error: `current_key` now returns key, not value Added `test_nested_key_val_for_loop` Added back include impl, error_location - Added `CallStack::error_location` - Added `StackFrame::template_location` - Split `find_value` into `find_value_(in_frame_context|in_for_loop)` - Added back `set_global` support - Cleaned `render_node` call `chain_err` Added back renderer tests - Updated `test_error_in_macro_location` error expectation - Updated `test_error_macros_self_inexisting` error expectation Patch `test_error_in_parent_template_location` expectations For literal array - store value in Array, not RefOrOwned - `add_set_values_in_context` working Added `should_break_for_loop` - Makes `render_for` test pass Added `out` macro to toggle `info` vs `println` - Added back `do_super` - Added `template` and `template_root` to AstProcessor - Added `MAGICAL_DUMP_VAR` support - Added Include FrameType and `push_include_frame` Extend lifetime of `context_value` Added two big context benchmarks Fix unused warning in `ref_or_owned` Add `find_value_by_path` Added `path_processor` - Fixed lifetimes on `find_value_...` functions - Added `Accessor` impl for `CallStack` - Update `AstProcessor` to use process_path for var processing - Back to original benches, with one fix to `huge_loop` - Removed `square_brackets`, covered by `path_processor` Fixed `MacroCollection` to track namespace->file - Fixed `eval_macro_call` to lookup macro in macro collection - Renamed `from_template_root` -> `from_template_original` Relaxed some error tests - Removed unnecessary map creation of macro definition map - Added `macto_namespace` to StrackFrame for macro lookup - Clippy suggestions - Used original `get_error_location` - Added tests for path processing Added `bench_macro_big_object_no_loop_{set|macro_call}` Fix remaining tests, relaxing text compare on failure Added big no loop samples.

- Removed references to stopwatch - Added missing assert in `error_unknown_index_variable`

…ess_clone

Keats

I tried to do the rebase myself but it's harder than just rebasing since string concat has been added to tera and this code need to handle it.

I don't think i will have the time this weekend or next week but I should be able to review it the week after (hopefully).

Keats · 2018-08-24T20:54:55Z

src/renderer/context.rs

+use context::get_json_pointer;
+use serde_json::value::Value;
+
+// --- module struct definitions ---


sorry I meant to remove these comments in every files

Ok, removed those.

…nchmarks-rs)

@patefacio

This is ported from @patefacio PR: #321 All credits go to him

Keats · 2018-09-01T09:44:48Z

Since the diff was so big, I ended up rewriting this PR to make sure I understood it well enough to maintain the codebase properly: the result is in #327

A few comments:

RefOrOwned exists in the std as Cow
it uncovered a few bugs with macros that I fixed but still need some digging/additional tests
the new structure is WAY cleaner than the old one: much easier to understand where we are not to mention allocating way less

I need to check if there are other places I can avoid allocating but overall thanks a lot for that PR, I marked you as author of #327 as it is mostly your work after all.

patefacio · 2018-09-02T12:26:59Z

This is great! Thanks. Someone recommended COW when I described the use-case and when I read its docs it sounded very different than my needs. So, it will be interesting to see how you used it.

…

On Sat, Sep 1, 2018 at 4:44 AM Vincent Prouillet ***@***.***> wrote: Since the diff was so big, I ended up rewriting this PR to make sure I understood it well enough to maintain the codebase properly: the result is in #327 <#327> A few comments: - RefOrOwned exists in the std as Cow <https://doc.rust-lang.org/stable/std/borrow/enum.Cow.html> - it uncovered a few bugs with macros that I fixed but still need some digging/additional tests - the new structure is WAY cleaner than the old one: much easier to understand where we are not to mention allocating way less I need to check if there are other places I can avoid allocating but overall thanks a lot for that PR, I marked you as author of #327 <#327> as it is mostly your work after all. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#321 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABC7PPPhOmyLLqkGr0WvjRtyJ9Hc4fC5ks5uWlcRgaJpZM4WIZlv> .

Keats · 2018-09-02T19:18:58Z

Someone recommended COW when I described the use-case and when I read its docs it sounded very different than my needs.

I do think it is the same overall, the main difference is that you can't add your own methods to it but it's not a big deal.

patefacio added 20 commits June 3, 2018 14:58

Added more success test cases

4e8a724

- `test_json_pointer` - `test_simple_for_loop` - `test_simple_nested_for_loop` - `test_simple_assignment` - `test_nested_for_var_access` - `test_error_render_field_unknown_in_forloop` to match error message

Added test_simple_key_val_for_loop

3533c30

- Fixed copy/paste error: `current_key` now returns key, not value

Added test_nested_key_val_for_loop

7215473

Added back include impl, error_location

90d334f

- Added `CallStack::error_location` - Added `StackFrame::template_location` - Split `find_value` into `find_value_(in_frame_context|in_for_loop)` - Added back `set_global` support - Cleaned `render_node` call `chain_err`

Added back renderer tests

eef3119

- Updated `test_error_in_macro_location` error expectation - Updated `test_error_macros_self_inexisting` error expectation

Patch test_error_in_parent_template_location expectations

22b5407

For literal array - store value in Array, not RefOrOwned

5f6b96e

- `add_set_values_in_context` working

Added should_break_for_loop

5b1b826

- Makes `render_for` test pass

Added out macro to toggle info vs println

3db7d5b

- Added back `do_super` - Added `template` and `template_root` to AstProcessor - Added `MAGICAL_DUMP_VAR` support - Added Include FrameType and `push_include_frame`

Extend lifetime of context_value

f2e4da1

Added two big context benchmarks

1c485d9

Fix unused warning in ref_or_owned

02ef774

Add find_value_by_path

91a5a91

Added path_processor

da19739

- Fixed lifetimes on `find_value_...` functions - Added `Accessor` impl for `CallStack` - Update `AstProcessor` to use process_path for var processing - Back to original benches, with one fix to `huge_loop` - Removed `square_brackets`, covered by `path_processor`

Fixed MacroCollection to track namespace->file

483f820

- Fixed `eval_macro_call` to lookup macro in macro collection - Renamed `from_template_root` -> `from_template_original`

Relaxed some error tests

0ae8c47

- Removed unnecessary map creation of macro definition map - Added `macto_namespace` to StrackFrame for macro lookup - Clippy suggestions - Used original `get_error_location` - Added tests for path processing

Added bench_macro_big_object_no_loop_{set|macro_call}

e2ff64f

Fix remaining tests, relaxing text compare on failure

dbe0619

Added big no loop samples.

f1a2970

Keats requested changes Aug 23, 2018

View reviewed changes

patefacio added 4 commits August 24, 2018 13:32

Removed module comments

53025c8

- Removed references to stopwatch - Added missing assert in `error_unknown_index_variable`

Merge branch 'less_clone' of https://github.com/patefacio/tera into l…

f4652c5

…ess_clone

Removed big_context_no_loop which was binary later moved to bench.

ef0c22d

Keats requested changes Aug 24, 2018

View reviewed changes

Remove module declaration comments

4dadd32

Keats pushed a commit that referenced this pull request Aug 25, 2018

Add more benchmarks (from #321 and https://github.com/djc/template-be…

1677a77

…nchmarks-rs)

Keats pushed a commit that referenced this pull request Aug 25, 2018

Add more benchmarks (from #321 and https://github.com/djc/template-be…

e5cbd13

…nchmarks-rs)

Keats pushed a commit that referenced this pull request Sep 1, 2018

Rewrite renderer to use Cows

02daaf6

This is ported from @patefacio PR: #321 All credits go to him

Keats mentioned this pull request Sep 1, 2018

Rewrite renderer to use Cows #327

Merged

Keats closed this Sep 1, 2018

naturallymitchell mentioned this pull request Feb 10, 2019

Crashing Issue when extending from unexisting template jazzdotdev/jazz#214

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance enhancement reducing clone #321

Performance enhancement reducing clone #321

patefacio commented Aug 22, 2018

Keats left a comment

Keats Aug 23, 2018

Keats Aug 23, 2018

patefacio Aug 24, 2018

Keats Aug 23, 2018

patefacio Aug 24, 2018

Keats Aug 23, 2018

patefacio Aug 24, 2018

Keats Aug 23, 2018

patefacio Aug 24, 2018

patefacio Aug 24, 2018

Keats left a comment

Keats Aug 24, 2018

patefacio Aug 24, 2018

Keats commented Sep 1, 2018

patefacio commented Sep 2, 2018 via email

Keats commented Sep 2, 2018 •

edited

Loading

		@@ -0,0 +1,1064 @@
		//! Responsible for walking ast and render implementation

		// --- module use statements ---

Performance enhancement reducing clone #321

Performance enhancement reducing clone #321

Conversation

patefacio commented Aug 22, 2018

Keats 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

Keats left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keats commented Sep 1, 2018

patefacio commented Sep 2, 2018 via email

Keats commented Sep 2, 2018 • edited Loading

Keats commented Sep 2, 2018 •

edited

Loading