Skip to content
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

Disabling RTTI #1718

Closed
hs-apotell opened this issue Aug 26, 2021 · 19 comments
Closed

Disabling RTTI #1718

hs-apotell opened this issue Aug 26, 2021 · 19 comments

Comments

@hs-apotell
Copy link
Collaborator

RTTI is enabled by default for both gcc & cl compiler. Also, dynamic_cast is being used across the codebase. Use of RTTI at library forces enabling RTTI at the program level which, ideally, we would like to disable it for number of reasons including performance and size.

There are options that can be used to restrict RTTI information to only the classes that need it. Here's an implementation. I am endorsing this implementation but just presenting one such option.

Any thoughts on implementing custom RTTI rather than forcing it to be enabled at program level?

P.S. We use CopperSpice (a fork of Qt) for our UI and the output binary sizes grow by a factor of 2.8 times by enabling RTTI at program level. This is not an ideal solution but UDHM/Surelog is the only library we use that forces enabling RTTI.

@alaindargelas
Copy link
Collaborator

Antlr is using it all over the place. No chance they'll change the code.
My fast version of Antlr did replace it manually in the most common places by bit fields. That works, it is painful but can be done.
On the other hand you are using the plain vanilla Antlr aren't you? So Surelog is not the only library that uses RTTI in your project.

@hzeller
Copy link
Collaborator

hzeller commented Aug 27, 2021

Could we just link RTTI inside the ANTLR library the the parser code it generates but switch it off for other object files ?
Of course the question is, outside of ANTLR, how feasible is it to not use RTTI in the rest of Surelog ?

Looks like there are only

find src -type f | grep -v /parser/ | xargs grep dynamic_cast | wc -l
223

I223 places, some UHDM, some other, they could probably be eliminated (also: exceptions outside the code that has to deal with Antlr that throws exceptions).

@hs-apotell
Copy link
Collaborator Author

On the other hand you are using the plain vanilla Antlr aren't you? So Surelog is not the only library that uses RTTI in your project.

No we are using the updated Antlr. The issue was that we wanted to use the updated version but it was part of Surelog and that meant pulling the whole repository which wasn't ideal. We just wanted Antlr pulled out of Surelog so dependencies are more explicit and clear.

Antlr is using it all over the place.

We will go thru' and update that too. We are looking at this very seriously because the generated code size differences. On Windows, the binaries file size differences are too big to be ignored.

If this group can agree on the need for this change and which specific RTTI implementation to use, we will take on the charge to make this change across the board.

@alaindargelas
Copy link
Collaborator

I'm OK with it, we might get more speed in Antlr if you use the same bitfield technique I used, if you use the kind of library you are pointing to, I'm not sure there will be speedups.
Now Antlr does use dynamic_casts in the generated code. You can't modify that by hand, every time we modify the grammar, you'd have to reedit the files under Surelog/src/parser/*.cc, that is not practical. You'd have to modify the Antlr parser generator or apply a script that modifies the generated parser as a post-processing.

@hzeller
Copy link
Collaborator

hzeller commented Aug 27, 2021

Before considering something that mimicks RTTI with some other library (which would always be a weird crutch), I'd consider not using RTTI in our code at all and only compile RTTI into the object files that deal with types (e.g. Antlr and its generated parser).

Is the codesize on windows dependent on if there is RTTI used at all, or is it proportional to the number of classes that need it ?
Can the linker find out at link time that an RTTI identifier is ever used on a class-hierarchy and strip it ?

@hs-apotell
Copy link
Collaborator Author

if you use the kind of library you are pointing to, I'm not sure there will be speedups.

The library I shared was just an example implementation. Here's performance comparison of using dynamic_cast vs. type_id solution.

You'd have to modify the Antlr parser generator or apply a script that modifies the generated parser as a post-processing.

We will evaluate the scope of work required to eliminate use of dynamic_cast all together. The work surely needs to be iterative.

I'd consider not using RTTI in our code at all and only compile RTTI into the object files that deal with types

AFAIK if any object file is compiled with RTTI information enabled, the linker expects it to be enabled all types in the final program. I don't know of a way to disable this.

Is the codesize on windows dependent on if there is RTTI used at all, or is it proportional to the number of classes that need it ?

Don't know off-and but can try and report back.

Can the linker find out at link time that an RTTI identifier is ever used on a class-hierarchy and strip it ?

That would be a kick-ass linker if it does all that reliably and consistently :)

hs-apotell added a commit to hs-apotell/Surelog that referenced this issue Sep 4, 2021
Surelog parsing is pretty slow

Resolution
Replacing use of standard RTTI i.e. use of dynamic_cast
with a custom implementation that avoid having to enable
RTTI across the entire application. This changes replaces
it for antlr only and future change would do the same
for whole of Surelog.

NOTE: The compile provided RTTI is not yet disabled.
hs-apotell added a commit to hs-apotell/Surelog that referenced this issue Sep 4, 2021
Surelog parsing is pretty slow

Resolution
Replacing use of standard RTTI i.e. use of dynamic_cast
with a custom implementation that avoid having to enable
RTTI across the entire application. This changes replaces
it for antlr only and future change would do the same
for whole of Surelog.

NOTE: The compile provided RTTI is not yet disabled.
alaindargelas added a commit that referenced this issue Sep 4, 2021
Issue #1718, #1727 Switch to using a custom implementation of RTTI (antlr specific for now)
@alaindargelas
Copy link
Collaborator

@hs-apotell any update/path to recoup the 24% runtime increase? If not we have to revert back to stock antlr (Not my fork) with RTTI in the main Surelog as the slowdown is not acceptable on Linux.
You can point to your RTTI-less Antlr implementation in your fork. For the antlr generated code in Surelog and the surelog code itself, I think we can use your RTTI replacement as the dynamic casts are not in the millions per run like in Antlr.
But first we need to recoup the 24% loss. Please update on your findings.

@hs-apotell
Copy link
Collaborator Author

Sorry, juggling multiple priorities. I haven't had chance to evaluate the performance of the solution just yet. As a stop gap solution, can we just point Surelog to use the master branch of Antlr instead of the modified Surelog branch. That would mean using pristine Antlr without the bitfield modifications. I will try get the RTTI version working in next few days. This is critical change for us as well which has to get done but at the immediate moment I need to clear my backlog so I can dedicate some exclusive time on this.

If you are comfortable with this stop gap solution, either drop a comment here so I can create the PR with necessary change or update the .gitmodules yourself.

@alaindargelas
Copy link
Collaborator

Yes please do create a PR with default main antlr

hs-apotell added a commit to hs-apotell/Surelog that referenced this issue Sep 8, 2021
Introduced RTTI solution seems to be slower than the standard
language implementation using dynamic_cast. There is no reason
for this to be the case and needs investigation. For the time
being, as a stop gap solution, falling back to pristine Antlr
to not have all developers pay the penalty.
alaindargelas added a commit that referenced this issue Sep 9, 2021
Issue #1718: Fallback to pristine antlr
@alaindargelas
Copy link
Collaborator

OK, after merging the stock Antlr here are the runtime measurements:

Stock Antlr runs the regression in 1980s
Antlr with RTTI removed using @hs-apotell's technique runs in 1660s
My version with the bitfields runs in 1306s.

I'm adding back the bitfield version under third_party/antlr_fast and make the Surelog cmake point to it meanwhile @hs-apotell experiments with the stock antlr/RTTI-less antlr.
When I ran the experiment a couple of days ago, maybe I was never running the stock Antlr, maybe I was running the antlr with the RTTI removed.

But one correction is that @hs-apotell's version is faster than the stock antlr, just not as fast as the bitfields.

@alaindargelas
Copy link
Collaborator

@hs-apotell Let's stop using this fork: https:/alainmarcel/antlr4
Either make a fork directly under chipsalliance or propose a new location since you are the one experimenting with stock antlr.
Re-add the antlr subsystem in Surelog when you have a working version that has a performance not too far from the bitfield version.

@hs-apotell
Copy link
Collaborator Author

hs-apotell commented Sep 9, 2021

I don't think I have permissions to fork under chipsalliance umbrella.

Optionally, why don't you just transfer the repository to the chipsalliance organization? That would keep the history, issues, and everything intact.

@alaindargelas
Copy link
Collaborator

@mgielda do you think we can have an antlr fork under ChipsAlliance. That way Surelog/UHDM won't rely on an aobscure fork in my alainmarcel org.

@hs-apotell
Copy link
Collaborator Author

@alaindargelas When you get a moment could you please try PR #1825 (or optionally use alainmarcel/antlr4#4)

Build times from chipsalliance/Surelog builds, averaging 2558s

Build | Time
-------------
2020 | 2257s
2001 | 2628s
1993 | 2712s
1989 | 2627s
1983 | 2570s

Build time from hs-apotell/Surelog build

Build | Time
------------
99 | 2436s

I am going to kick off a few more runs of the change to get a better average.

@alaindargelas
Copy link
Collaborator

@hs-apotell can you give me clear instructions on how to pull a draft PR #1825? I'm reading many posts, I can't make head or tail...

@hs-apotell
Copy link
Collaborator Author

hs-apotell commented Sep 11, 2021 via email

@alaindargelas
Copy link
Collaborator

alaindargelas commented Sep 11, 2021

@hs-apotell, that was not the question. I still didn't get the OK to transfer the alainmarcel/antlr to chipsalliance. That is on standby.

In the meantime, I need to know how to checkout an unmerged pull request. Do you know how?

@alaindargelas
Copy link
Collaborator

Got it!
git clone https:/chipsalliance/Surelog
cd Surelog
git submodule update --init --recursive
git fetch origin pull/1825/head:test-rtti
git submodule update --init --recursive
git checkout test-rtti

@alaindargelas
Copy link
Collaborator

It's still 1700s vs 1300s for the bitfield version.
As long as this is used only on Windows, I'm OK. Submit a complete PR that uses the third_party/antlr_fast for Linux and Macos and uses the third_party/antlr for Windows only. You can use your RTTI thing in the Surelog code too. That will not make a difference.

hs-apotell added a commit to hs-apotell/Surelog that referenced this issue Sep 13, 2021
commit 99c4684
Merge: f3573b5 3500608
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 22:13:35 2021 -0700

    Merge pull request chipsalliance#1852 from chipsalliance/alainmarcel-patch-1

    grammar opt

commit 3500608
Merge: 60e16eb 2c0a1be
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 22:13:15 2021 -0700

    Merge pull request chipsalliance#1851 from alainmarcel/alainmarcel-patch-1

    grammar opt

commit 2c0a1be
Author: Alain Dargelas <[email protected]>
Date:   Sun Sep 12 22:12:32 2021 -0700

    grammar opt

commit f3573b5
Merge: 41d7414 60e16eb
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 21:51:16 2021 -0700

    Merge pull request chipsalliance#1850 from chipsalliance/alainmarcel-patch-1

     grammar opt

commit 60e16eb
Merge: 99ec016 2c2c802
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 21:50:52 2021 -0700

    Merge pull request chipsalliance#1849 from alainmarcel/alainmarcel-patch-1

     grammar opt

commit 2c2c802
Author: Alain Dargelas <[email protected]>
Date:   Sun Sep 12 21:49:54 2021 -0700

    grammar opt

commit 41d7414
Merge: ad7d19c 99ec016
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 21:12:46 2021 -0700

    Merge pull request chipsalliance#1848 from chipsalliance/alainmarcel-patch-1

    grammar opt

commit 99ec016
Merge: 7f167cb 7d3528d
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 21:12:23 2021 -0700

    Merge pull request chipsalliance#1847 from alainmarcel/alainmarcel-patch-1

    grammar opt

commit 7d3528d
Author: Alain Dargelas <[email protected]>
Date:   Sun Sep 12 21:11:42 2021 -0700

    grammar opt

commit ad7d19c
Merge: 743ccaa 7f167cb
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 20:59:17 2021 -0700

    Merge pull request chipsalliance#1846 from chipsalliance/alainmarcel-patch-1

    grammar opt

commit 7f167cb
Merge: f014577 d3b7881
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 20:58:31 2021 -0700

    Merge pull request chipsalliance#1845 from alainmarcel/alainmarcel-patch-1

    grammar opt

commit d3b7881
Author: Alain Dargelas <[email protected]>
Date:   Sun Sep 12 20:57:19 2021 -0700

    grammar opt

commit 743ccaa
Merge: 6ccbe81 f014577
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 15:30:24 2021 -0700

    Merge pull request chipsalliance#1844 from chipsalliance/alainmarcel-patch-1

    vpiInstance relation for typespec

commit f014577
Merge: 8161f92 32fd977
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 15:29:50 2021 -0700

    Merge pull request chipsalliance#1843 from alainmarcel/alainmarcel-patch-1

    vpiInstance relation for typespec

commit 32fd977
Author: Alain Dargelas <[email protected]>
Date:   Sun Sep 12 15:29:06 2021 -0700

    vpiInstance relation for typespec

commit 6ccbe81
Merge: 9a13ce9 402a9b2
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 11:02:53 2021 -0700

    Merge pull request chipsalliance#1842 from hzeller/include-cache-and-open-in-gitignore

    Include autobackup and .cache in gitignore.

commit 402a9b2
Author: Henner Zeller <[email protected]>
Date:   Sun Sep 12 09:31:17 2021 -0700

    Include autobackup and .cache in gitignore.

    created by clangd

    Signed-off-by: Henner Zeller <[email protected]>

commit 9a13ce9
Merge: 07f4b8a 8161f92
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 09:26:22 2021 -0700

    Merge pull request chipsalliance#1841 from chipsalliance/alainmarcel-patch-1

    rm antlr dir

commit 8161f92
Merge: 4c9913a e86a432
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 09:25:45 2021 -0700

    Merge pull request chipsalliance#1840 from alainmarcel/alainmarcel-patch-1

    rm antlr

commit e86a432
Author: Alain Dargelas <[email protected]>
Date:   Sun Sep 12 09:21:58 2021 -0700

    rm antlr

commit 07f4b8a
Merge: 35b9cc5 4c9913a
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 00:32:31 2021 -0700

    Merge pull request chipsalliance#1834 from chipsalliance/alainmarcel-patch-1

    formatting

commit 4c9913a
Merge: 06d19b2 23b9b23
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 00:32:09 2021 -0700

    Merge pull request chipsalliance#1833 from alainmarcel/alainmarcel-patch-1

    formatting

commit 23b9b23
Author: Alain Dargelas <[email protected]>
Date:   Sun Sep 12 00:31:34 2021 -0700

    formatting

commit 35b9cc5
Merge: 180ffe1 06d19b2
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 00:17:24 2021 -0700

    Merge pull request chipsalliance#1831 from chipsalliance/alainmarcel-patch-1

    remove antlr submodule (tmp)

commit 06d19b2
Merge: 3f3b65a ac486ce
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 00:16:54 2021 -0700

    Merge pull request chipsalliance#1830 from alainmarcel/alainmarcel-patch-1

    remove antlr submodule

commit ac486ce
Author: Alain Dargelas <[email protected]>
Date:   Sun Sep 12 00:16:17 2021 -0700

    remove antlr submodule

commit 180ffe1
Merge: d310b26 3f3b65a
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 00:13:17 2021 -0700

    Merge pull request chipsalliance#1829 from chipsalliance/alainmarcel-patch-1

    elab and binding

commit 3f3b65a
Merge: eba317b c8f8412
Author: alaindargelas <[email protected]>
Date:   Sun Sep 12 00:12:44 2021 -0700

    Merge pull request chipsalliance#1828 from alainmarcel/alainmarcel-patch-1

    elab and binding

commit c8f8412
Merge: 313bd5b d10fdba
Author: Alain Dargelas <[email protected]>
Date:   Sun Sep 12 00:11:51 2021 -0700

    Merge branch 'alainmarcel-patch-1' of https:/alainmarcel/Surelog into alainmarcel-patch-1

commit 313bd5b
Author: Alain Dargelas <[email protected]>
Date:   Sun Sep 12 00:11:37 2021 -0700

    elab and binding

commit 3f3212b
Merge: 90fe7ec d310b26
Author: Alain Dargelas <[email protected]>
Date:   Sat Sep 11 23:48:54 2021 -0700

    Merge branch 'master' into alainmarcel-patch-1

commit d310b26
Merge: d682d51 45a1e6e
Author: alaindargelas <[email protected]>
Date:   Sat Sep 11 23:46:43 2021 -0700

    Merge pull request chipsalliance#1827 from hzeller/uhdm-includes

    Change uhdm includes to use <uhdm/...> standard location.

commit 45a1e6e
Author: Henner Zeller <[email protected]>
Date:   Sat Sep 11 22:30:00 2021 -0700

    Change uhdm includes to use <uhdm/...> standard location.

    Signed-off-by: Henner Zeller <[email protected]>

commit 90fe7ec
Merge: ccd7b78 d682d51
Author: Alain Dargelas <[email protected]>
Date:   Sat Sep 11 22:00:14 2021 -0700

    Merge branch 'master' into alainmarcel-patch-1

commit d10fdba
Merge: 9093c39 eba317b
Author: alaindargelas <[email protected]>
Date:   Sat Sep 11 21:58:43 2021 -0700

    Merge branch 'chipsalliance:alainmarcel-patch-1' into alainmarcel-patch-1

commit d682d51
Merge: a65c563 eba317b
Author: alaindargelas <[email protected]>
Date:   Sat Sep 11 21:58:12 2021 -0700

    Merge pull request chipsalliance#1826 from chipsalliance/alainmarcel-patch-1

    UHDM sync

commit eba317b
Author: Alain Dargelas <[email protected]>
Date:   Sat Sep 11 21:57:08 2021 -0700

    UHDM sync

commit ccd7b78
Merge: 9093c39 a65c563
Author: Alain Dargelas <[email protected]>
Date:   Sat Sep 11 14:09:55 2021 -0700

    Merge branch 'master' into alainmarcel-patch-1

commit a65c563
Merge: ff28a2e f28cd4b
Author: alaindargelas <[email protected]>
Date:   Sat Sep 11 14:09:03 2021 -0700

    Merge pull request chipsalliance#1824 from hzeller/prepare-for-uhdm-update

    Make uhdm includes qualified within uhdm-directory.

commit f28cd4b
Author: Henner Zeller <[email protected]>
Date:   Sat Sep 11 01:09:51 2021 -0700

    Make uhdm includes qualified within uhdm-directory.

    As preparation to switch to latest UHDM head.

    Signed-off-by: Henner Zeller <[email protected]>

commit ff28a2e
Merge: 2122779 098a21a
Author: alaindargelas <[email protected]>
Date:   Thu Sep 9 07:46:43 2021 -0700

    Merge pull request chipsalliance#1823 from antmicro/remove-macos-patch

    Remove flatbuffers patch

commit 098a21a
Author: Rafal Kapuscik <[email protected]>
Date:   Thu Sep 9 08:40:07 2021 +0200

    Remove flatbuffers patch

    Signed-off-by: Rafal Kapuscik <[email protected]>

commit 9093c39
Author: Alain Dargelas <[email protected]>
Date:   Wed Sep 8 22:50:14 2021 -0700

    re-adding antlr

commit 2122779
Merge: 0f789c5 6ff1331
Author: alaindargelas <[email protected]>
Date:   Wed Sep 8 22:40:13 2021 -0700

    Merge pull request chipsalliance#1821 from chipsalliance/alainmarcel-patch-1

    temporarily remove antlr submodule

commit 6ff1331
Merge: a41441f 7d8a654
Author: alaindargelas <[email protected]>
Date:   Wed Sep 8 22:39:32 2021 -0700

    Merge pull request chipsalliance#1820 from alainmarcel/alainmarcel-patch-1

    temporarily remove antlr submodule

commit 7d8a654
Author: Alain Dargelas <[email protected]>
Date:   Wed Sep 8 22:36:11 2021 -0700

    antlr git issue

commit 0f789c5
Merge: fce00c6 a41441f
Author: alaindargelas <[email protected]>
Date:   Wed Sep 8 22:18:09 2021 -0700

    Merge pull request chipsalliance#1819 from chipsalliance/alainmarcel-patch-1

    Making the antlr_fast the default antlr for now

commit a41441f
Merge: 215f668 2a5db10
Author: alaindargelas <[email protected]>
Date:   Wed Sep 8 22:17:45 2021 -0700

    Merge pull request chipsalliance#1818 from alainmarcel/alainmarcel-patch-1

    Making the antlr_fast the default antlr for now

commit 2a5db10
Author: Alain Dargelas <[email protected]>
Date:   Wed Sep 8 22:16:19 2021 -0700

    Making the antlr_fast the default antlr for now

commit fce00c6
Merge: 82ea885 215f668
Author: alaindargelas <[email protected]>
Date:   Wed Sep 8 21:52:18 2021 -0700

    Merge pull request chipsalliance#1817 from chipsalliance/alainmarcel-patch-1

    Fast Antlr with bitfields instead of dynamic_cast

commit 215f668
Merge: fc31994 6a967a6
Author: alaindargelas <[email protected]>
Date:   Wed Sep 8 21:51:51 2021 -0700

    Merge pull request chipsalliance#1816 from alainmarcel/alainmarcel-patch-1

    Fast Antlr with bitfields instead of dynamic_cast

commit 6a967a6
Author: Alain Dargelas <[email protected]>
Date:   Wed Sep 8 21:47:49 2021 -0700

    Fast Antlr with bitfields instead of dynamic_cast

commit 82ea885
Merge: 6d8d64c ba7e8e3
Author: alaindargelas <[email protected]>
Date:   Wed Sep 8 20:29:07 2021 -0700

    Merge pull request chipsalliance#1815 from hs-apotell/fallback

    Issue chipsalliance#1718: Fallback to pristine antlr

commit ba7e8e3
Author: HS <[email protected]>
Date:   Wed Sep 8 13:01:48 2021 -0700

    Pulling prebuilt binary from Surelog branch doesn't work since the
    changes in that branch aren't portable. Rebuilt the binary.

commit db9c97c
Author: HS <[email protected]>
Date:   Wed Sep 8 12:48:56 2021 -0700

    Include the Surelog specific changes as well

commit c9f6a42
Author: HS <[email protected]>
Date:   Wed Sep 8 12:31:17 2021 -0700

    Issue chipsalliance#1718: Fallback to pristine antlr

    Introduced RTTI solution seems to be slower than the standard
    language implementation using dynamic_cast. There is no reason
    for this to be the case and needs investigation. For the time
    being, as a stop gap solution, falling back to pristine Antlr
    to not have all developers pay the penalty.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants