p4lang / p4c

P4_16 reference compiler
https://p4.org/
Apache License 2.0
683 stars 446 forks source link

Replace Util::Enumerator with a C++-native enumerator in resolveReferences #4412

Open fruffy opened 9 months ago

fruffy commented 9 months ago

I suspect you could get a much bigger gain here by getting rid of the java-style Util::Enumerator (which does a bunch of dynamic allocation and generates much garbage to be collected by the GC) and using C++ style enumeration instead, but that would require rewriting all the code that uses this (many passes). One could think about having a second interface in a ResolutionContext that allows for that and then incrementally change other code to use the new interface. But that is a bigger project,

Context: https://github.com/p4lang/p4c/pull/4376#discussion_r1485685582

vlstill commented 9 months ago

I think that ideally, we would replace a lot of these with C++ ranges (although they are not a direct replacement in many cases I guess -- partially because Util::Enumerator does type erasure). But that would depend raising compiler requirement for building p4c. I'm not sure who are the bounding users (and if we even know). Ubuntu 20.04 LTS has 10.5, which should have working ranges (I haven't tested/don't know if there are any bugs). 18.04LTS has 7.4, far older then the required 9. So I am not sure where is the GCC 9 requirement coming from. Might be RHEL 8, which has GCC 9, but maybe if that is the case we could ask users to use https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/developing_c_and_cpp_applications_in_rhel_8/additional-toolsets-for-development_developing-applications#gcc-toolset_assembly_additional-toolsets-for-development there (yes I know RHEL 8 is not officially supported by p4c but presumably it would be quite common in enterprise).

The other important question is if the C++20 support is stable enough on the given compiler.

An alternative would be to use a 3rd party library that is similar to C++ ranges and support GCC 9, but I am not sure if there is a good candidate.

asl commented 9 months ago

(although they are not a direct replacement in many cases I guess -- partially because Util::Enumerator does type erasure).

Yes. I think the modelling example here is getDeclarations. The bad thing about Enumerator itself is that various adapters (where / concat) does lots of allocations for no particular reason. This would be replaced with views ideally (though I remember there were some performance issues with concat in ranges-v3).

For type erasure we might consider replacing with value semantics (Model / Concept-based polymorphism), though It's likely will be similar wrt allocations and overheads...

The other important question is if the C++20 support is stable enough on the given compiler.

Well... that's another story, yes. There are lots of small issues and corner cases. E.g. in order to build LLVM with C++20 enabled one needs to have clang 17.0.6.