Refactor parser #346

Adda0 · 2023-09-26T07:32:13Z

This PR improves the performance of parser. More optimizations are to come, as well as a possible rewrite of the formula parser.

codecov · 2023-09-26T07:40:02Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (069554c) 72.94% compared to head (ef48e39) 72.95%.
Report is 1 commits behind head on devel.

Additional details and impacted files

@@           Coverage Diff           @@
##            devel     #346   +/-   ##
=======================================
  Coverage   72.94%   72.95%           
=======================================
  Files          33       33           
  Lines        4144     4148    +4     
  Branches      955      955           
=======================================
+ Hits         3023     3026    +3     
- Misses        740      741    +1     
  Partials      381      381

Files	Coverage Δ
include/mata/parser/inter-aut.hh	`97.29% <100.00%> (ø)`
src/parser.cc	`81.61% <100.00%> (ø)`
src/inter-aut.cc	`68.36% <86.48%> (+0.07%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jurajsic · 2023-09-26T14:33:19Z

src/inter-aut.cc

                    for (int j = static_cast<int>(opstack.size())-1; j >= 0; --j) {
-                        size_t j_size_t{ static_cast<size_t>(j) };
-                        assert(!opstack[j_size_t].is_operand());
-                        if (opstack[j_size_t].is_leftpar())
+                        auto formula_node_opstack_it{ opstack.begin() + j };


Isn't this just iterating from end to begin? So we could have
for (auto formula_node_opstack_it = opstack.rbegin(); formula_node_opstack_it != opstack.rend(); ++formula_node_opstack_it)?

I think you are right. It should be possible to refactor this as you say. I will investigate.

The int arithmetic is there so you can do erase() in the vector. It will not work with a reverse iterator. I would keep it as it is, or rework the logic to not have to erase inside the loop.

I think it could be done somehow with erase(), because it returns iterator, but a forward one.
But moving erasing outside the loop would probably be the best.

kilohsakul · 2023-09-28T22:16:54Z

include/mata/parser/inter-aut.hh

    FormulaNode node{};
    std::vector<FormulaGraph> children{};

    FormulaGraph() = default;
-    FormulaGraph(const FormulaNode& n) : node(n), children() {}
-    FormulaGraph(FormulaNode&& n) : node(std::move(n)), children() {}
+    explicit FormulaGraph(const FormulaNode& n) : node(n), children() { children.reserve(2); }


just wandering, can you put the two alredy to the the line 129 with the declaration?

No, vector has no such constructor, unfortunately. It can be done with a lambda or a named function inside the parentheses in the initialization list. Something like

FormulaGraph() : children([](){ std::vector<FormulaNode> children; children.reserve(100); return children;}()) {}

kilohsakul · 2023-09-28T22:18:55Z

src/parser.cc

        if (token.second) { // is quoted?
            result.push_back(std::move(token));
            continue;
        }

-        const std::string_view token_string = token.first;
-        size_t last_operator = 0;
+        const std::string_view token_string{ token.first };


why this everywhere actually?

I mena why to initialize things with the curly brackets {bla} instead of = bla?

I think it is some C++ idiom, that is advised to use. I myself don't like it.

It is to prevent narrowing of the types (https://stackoverflow.com/a/27755143), but naturally, C++ community has to make sure it is ugly as pug's arse.

Exactly as you say. It is a good idea, only horribly implemented as far as visualization goes. But it helps indeed and warns you about potential implicit conversion bugs. In modern C++, it should always be preferred over assignment operator.

kilohsakul · 2023-09-28T22:21:02Z

anyway, great!

kilohsakul · 2023-09-29T07:04:52Z

src/parser.cc

        if (token.second) { // is quoted?
            result.push_back(std::move(token));
            continue;
        }

-        const std::string_view token_string = token.first;
-        size_t last_operator = 0;
+        const std::string_view token_string{ token.first };


I mena why to initialize things with the curly brackets {bla} instead of = bla?

kilohsakul · 2023-09-29T07:30:01Z

Weird thing, if you look at the checks - the performance tests, then you can see that complement got faster.
It should not, right?
Also the comparison of times for operations do not seem to correspond to differences in runtimes. Maybe it would deserve little investigation eventually (not within this pull request).

include/mata/parser/inter-aut.hh

tfiedor · 2023-09-29T12:37:18Z

src/parser.cc

        if (token.second) { // is quoted?
            result.push_back(std::move(token));
            continue;
        }

-        const std::string_view token_string = token.first;
-        size_t last_operator = 0;
+        const std::string_view token_string{ token.first };


I think it is some C++ idiom, that is advised to use. I myself don't like it.

tfiedor · 2023-09-29T12:38:52Z

src/parser.cc

        if (token.second) { // is quoted?
            result.push_back(std::move(token));
            continue;
        }

-        const std::string_view token_string = token.first;
-        size_t last_operator = 0;
+        const std::string_view token_string{ token.first };


It is to prevent narrowing of the types (https://stackoverflow.com/a/27755143), but naturally, C++ community has to make sure it is ugly as pug's arse.

tfiedor · 2023-09-29T19:04:26Z

The checks are passing, we decided to move with this PR. Please @Adda0 check the rest of the conversations post mortem. Oaw oaw, thanks.

Adda0 · 2023-10-02T06:05:11Z

Weird thing, if you look at the checks - the performance tests, then you can see that complement got faster.
It should not, right?
Also the comparison of times for operations do not seem to correspond to differences in runtimes. Maybe it would deserve little investigation eventually (not within this pull request).

Complement got slower, not faster. I think this is only a deviation which stems from us running the performance tests only once. The runtimes are not precise, and it runs faster one time and slower another time. Nothing except for parsing and mintermization can change in this PR.

Adda0 · 2023-10-02T06:08:30Z

The changes from the discussions were addressed and will be part of a future parser optimization PR.

Adda0 added 7 commits September 26, 2023 08:17

Reserve space for children

b1d52c6

Fix formatting

536d4e9

Apply moves on tokens

08cf090

Optimize section parsing

47e0c86

Omit creating local variable when unnecessary

3c05412

Fix move constructors by adding noexcept

d93ed34

Use iterator for quicker erase

ef48e39

Adda0 requested review from tfiedor and jurajsic September 26, 2023 07:32

jurajsic approved these changes Sep 26, 2023

View reviewed changes

kilohsakul reviewed Sep 28, 2023

View reviewed changes

kilohsakul approved these changes Sep 28, 2023

View reviewed changes

kilohsakul approved these changes Sep 29, 2023

View reviewed changes

tfiedor approved these changes Sep 29, 2023

View reviewed changes

tfiedor merged commit bb85433 into devel Sep 29, 2023
20 checks passed

tfiedor deleted the refactor_parser branch September 29, 2023 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor parser #346

Refactor parser #346

Adda0 commented Sep 26, 2023

codecov bot commented Sep 26, 2023

jurajsic Sep 26, 2023 •

edited

Loading

Adda0 Sep 27, 2023

Adda0 Sep 27, 2023 •

edited

Loading

jurajsic Sep 27, 2023

kilohsakul Sep 28, 2023

Adda0 Oct 2, 2023

kilohsakul Sep 28, 2023

kilohsakul Sep 29, 2023

tfiedor Sep 29, 2023

tfiedor Sep 29, 2023

Adda0 Oct 2, 2023

kilohsakul commented Sep 28, 2023

kilohsakul Sep 29, 2023

kilohsakul commented Sep 29, 2023

tfiedor Sep 29, 2023

tfiedor Sep 29, 2023

tfiedor commented Sep 29, 2023

Adda0 commented Oct 2, 2023 •

edited

Loading

Adda0 commented Oct 2, 2023 •

edited

Loading

Refactor parser #346

Refactor parser #346

Conversation

Adda0 commented Sep 26, 2023

codecov bot commented Sep 26, 2023

Codecov Report

jurajsic Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adda0 Sep 27, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kilohsakul commented Sep 28, 2023

Choose a reason for hiding this comment

kilohsakul commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfiedor commented Sep 29, 2023

Adda0 commented Oct 2, 2023 • edited Loading

Adda0 commented Oct 2, 2023 • edited Loading

jurajsic Sep 26, 2023 •

edited

Loading

Adda0 Sep 27, 2023 •

edited

Loading

Adda0 commented Oct 2, 2023 •

edited

Loading

Adda0 commented Oct 2, 2023 •

edited

Loading