FWD » Core Development » Conversion Tools

Octavian: Let's go ahead with #11317-3 immediately, without addressing the larger threading issue.

Created task branch 11317a.

The large customer application completed successfully. I got 3h13m with 11317a and 2h54m with trunk/16731. There are no differences between the generated outputs. I ran both using 16 threads and a 24G conversion heap. Here are my machine's specifications: ShowHide

Architecture:                x86_64
  CPU op-mode(s):            32-bit, 64-bit
  Address sizes:             48 bits physical, 48 bits virtual
  Byte Order:                Little Endian
CPU(s):                      12
  On-line CPU(s) list:       0-11
Vendor ID:                   AuthenticAMD
  Model name:                AMD Ryzen 5 7530U with Radeon Graphics
    CPU family:              25
    Model:                   80
    Thread(s) per core:      2
    Core(s) per socket:      6
    Socket(s):               1
    Stepping:                0
    Frequency boost:         enabled
    CPU(s) scaling MHz:      68%
    CPU max MHz:             4547.9458
    CPU min MHz:             410.9590
    BogoMIPS:                3992.65
    Flags:                   fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat
                              pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pd
                             pe1gb rdtscp lm constant_tsc rep_good nopl xtopology nonstop_tsc 
                             cpuid extd_apicid aperfmperf rapl pni pclmulqdq monitor ssse3 fma
                              cx16 sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand lahf_l
                             m cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowpr
                             efetch osvw ibs skinit wdt tce topoext perfctr_core perfctr_nb bp
                             ext perfctr_llc mwaitx cpb cat_l3 cdp_l3 hw_pstate ssbd mba ibrs 
                             ibpb stibp vmmcall fsgsbase bmi1 avx2 smep bmi2 erms invpcid cqm 
                             rdt_a rdseed adx smap clflushopt clwb sha_ni xsaveopt xsavec xget
                             bv1 xsaves cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local user
                             _shstk clzero irperf xsaveerptr rdpru wbnoinvd cppc arat npt lbrv
                              svm_lock nrip_save tsc_scale vmcb_clean flushbyasid decodeassist
                             s pausefilter pfthreshold avic v_vmsave_vmload vgif v_spec_ctrl u
                             mip pku ospke vaes vpclmulqdq rdpid overflow_recov succor smca fs
                             rm debug_swap
Virtualization features:     
  Virtualization:            AMD-V
Caches (sum of all):         
  L1d:                       192 KiB (6 instances)
  L1i:                       192 KiB (6 instances)
  L2:                        3 MiB (6 instances)
  L3:                        16 MiB (1 instance)
NUMA:                        
  NUMA node(s):              1
  NUMA node0 CPU(s):         0-11
Vulnerabilities:             
  Gather data sampling:      Not affected
  Ghostwrite:                Not affected
  Indirect target selection: Not affected
  Itlb multihit:             Not affected
  L1tf:                      Not affected
  Mds:                       Not affected
  Meltdown:                  Not affected
  Mmio stale data:           Not affected
  Old microcode:             Not affected
  Reg file data sampling:    Not affected
  Retbleed:                  Not affected
  Spec rstack overflow:      Mitigation; Safe RET
  Spec store bypass:         Mitigation; Speculative Store Bypass disabled via prctl
  Spectre v1:                Mitigation; usercopy/swapgs barriers and __user pointer sanitizat
                             ion
  Spectre v2:                Mitigation; Retpolines; IBPB conditional; IBRS_FW; STIBP always-o
                             n; RSB filling; PBRSB-eIBRS Not affected; BHI Not affected
  Srbds:                     Not affected
  Tsa:                       Vulnerable: No microcode
  Tsx async abort:           Not affected
  Vmscape:                   Mitigation; IBPB before exit to userspace

The time difference was unexpected, so I checked for potential causes. I found that the conversion with 11317a ran with the debug agent enabled, which likely explains the variance. I'll rerun the conversion to confirm.

I ran both conversions again. The one with 11317a took 2h56m, and the one with trunk took 2h52m. The results are more expected now. There's still no noticeable performance improvement, but the execution times are virtually the same.

I'll start experimenting based on your ideas.

As a side idea, I wonder if we can keep a static cache of ASTs to be reused directly from memory. So, instead of flushing to disk and reading again, to have a small cache in-memory to keep the most used ASTs (schema ASTs I suppose are one of them, but super-classes used anywhere can fit as well). We need to avoid thrashing such cache with one-time asts, so maybe we can also persist a "hit count" and if the hit count exceeds a threshold to also save the ASTs in a memory registry.

Don't force the cache now. It may be a small gain. The ReadWriteReentrantLock may be major.

Some context on a similar project as the one above.

• Parser: 8m32s (I know this is not multi-threaded)
• Post-parse fix-ups: 47s (can't tell why this is so slow)
• Early annotations: 2m09s -> this was the phase from which I extracted the first image above
• Gap analysis: 15s
• Schema fix-ups: 2s
• Schema annotations: 6s
• Preprocess P2O: 5s
• P2O Generation: 13s
• P2O post-processing: 2s
• Generate DMO: 10s
• Generate Java DMO: 11s
• Unreachable Code 10s
• Code Conversion Anno Prep: 17s
• Code Conversion Annotations: 10m53s -> this was the phase from which I extracted the second image above
• Frame generator: 13s
• Business Logic Base Structure: 28s
• Core Code Conversion: 2m2s -> this is all green, no locking. It is pure TRPL work (most threads are in RuleContainer.apply)
• Generate Java: 19s

I may have missed some phases, but they were most probably < 10s.

I have tested the changes with ReentrantReadWriteLock, but it's really hard to see any noticeable improvement. I think performance strongly depends on the project and how it is structured, as we might be dealing with more dependencies between artifacts.

Alex, could you provide the Utilizing N concurrent thread(s) to process M file(s); K CPU(s), H MiB heap log message if it is still available?

Utilizing 16 concurrent thread(s) to process 6800 file(s); 16 CPU, 16384 MiB heap.

This is what I have.

I have tested the changes with ReentrantReadWriteLock, but it's really hard to see any noticeable improvement. I think performance strongly depends on the project and how it is structured, as we might be dealing with more dependencies between artifacts.

Feel free to commit the changes, I can test them and let you know if I see an improvement on it.

Alexandru Lungu wrote:

Feel free to commit the changes, I can test them and let you know if I see an improvement on it.

Created task branch 11317b.

I've committed the changes in 11317b/16762.

• I am not entirely sure AnnotatedAst.java change is reasonable. Please do a bzr blame to check the author of the Collections.synchronizedMap(new IdentityHashMap<>()) data structure in the first place. The goal of the intern was exactly to force == comparison instead of .equals. As you run ConcurrentHashMap and String lower = ANNO_LOWER.get(anno);, the anno not being internalized, it will be often compared with equals character by character. Please check the OG task that added IdentityHashMap and add the author as a watcher to review. I would prefer some benchmark tho for this decision (16 threads thrashing such data structure).
  • TBH, a synchronized collection here might be a dead weight. I can't understand why a simple toLowerCase won't work faster in a multi-threaded environment, then a synchronized structure. The annotation keys are short and ASCII usually, so paying a monitor price seems too much.

• loadTreeHeader is a reasonable change.

+      if (ast == null)
+      {
+         // no class or interface definition in there, nothing to do
+         return;
+      }

• this is a new path that was not considered before. Isn't this a fatal case that should throw?
  • Or log?
  • Or fallback to Aast ast = mgr.loadTree(artifact);?
• Also, the upcoming Aast def = ast.getImmediateChild(ProgressParserTokenTypes.CLASS_DEF, null); assumes that the header reads classes, but I guess it can also read interfaces according to the header reading implementation.
• CLASS_TREE_POSTFIX = ".cls.ast"; assumes that an artifact ending with .cls.ast is an OO class AST. I think this is correct, but this should be tested (can a class be defined in a .p.ast or something like .class.ast? I guess not, I never saw this, but such hard-coded value should be rather confirmed.
  • I would have liked something like CLASS_POSTFIX + . + AST_POSTFIX. AFAIK, the AST_POSTFIX can be configurable (?)
• Please compress the history entries to keep them usually one-line; any extra information should rather go in javadocs.
• The code in CommonAstSupport.java is no longer synchronized. Please triple check that all the users are properly locked and there are no other synchronized or synchronize(this) in the class or sub-class or a synchronized (support) (instance wide locking) that could share the same monitor.

Alexandru Lungu wrote:

• I am not entirely sure AnnotatedAst.java change is reasonable. Please do a bzr blame to check the author of the Collections.synchronizedMap(new IdentityHashMap<>()) data structure in the first place. The goal of the intern was exactly to force == comparison instead of .equals. As you run ConcurrentHashMap and String lower = ANNO_LOWER.get(anno);, the anno not being internalized, it will be often compared with equals character by character. Please check the OG task that added IdentityHashMap and add the author as a watcher to review. I would prefer some benchmark tho for this decision (16 threads thrashing such data structure).

The behaviour is the same as with trunk: the returned key is still the canonical, interned instance for every annotation. It is faster only because the lookup into ANNO_LOWER no longer has to intern its argument on every call and interning now happens once, when the entry is first stored. The canonical instance is what annotations needs, since that one is an IdentityHashMap. The cache in front of it does not have to be an IdentityHashMap as well.

Constantin managed this area in 8363g. Constantin, what do you think?

• TBH, a synchronized collection here might be a dead weight. I can't understand why a simple toLowerCase won't work faster in a multi-threaded environment, then a synchronized structure. The annotation keys are short and ASCII usually, so paying a monitor price seems too much.

I think that toLowerCase would make getAnnotationKey return non-canonical keys and there will be additional entries in annotations map since it's an IdentityHashMap.

• loadTreeHeader is a reasonable change.

[...]

• this is a new path that was not considered before. Isn't this a fatal case that should throw?
  • Or log?
  • Or fallback to Aast ast = mgr.loadTree(artifact);?

This path isn't new. The old implementation silently returned in the same case, just further down. It's related to the next review item.

• Also, the upcoming Aast def = ast.getImmediateChild(ProgressParserTokenTypes.CLASS_DEF, null); assumes that the header reads classes, but I guess it can also read interfaces according to the header reading implementation.

Yes, it does, but further down, after the header is read:

      Aast def    = ast.getImmediateChild(ProgressParserTokenTypes.CLASS_DEF, null);
      Aast header = null;

      if (def == null)
      {
         def = ast.getImmediateChild(ProgressParserTokenTypes.INTERFACE_DEF, null);

         if (def == null)
         {
            // must be an enum, nothing to do
            return;
         }

• CLASS_TREE_POSTFIX = ".cls.ast"; assumes that an artifact ending with .cls.ast is an OO class AST. I think this is correct, but this should be tested (can a class be defined in a .p.ast or something like .class.ast? I guess not, I never saw this, but such hard-coded value should be rather confirmed.
  • I would have liked something like CLASS_POSTFIX + . + AST_POSTFIX. AFAIK, the AST_POSTFIX can be configurable (?)
• Please compress the history entries to keep them usually one-line; any extra information should rather go in javadocs.

I fixed these in 11317b/16764. I also replaced the hard-coded value from TransforDriver as well and used configured file extensions.

• The code in CommonAstSupport.java is no longer synchronized. Please triple check that all the users are properly locked and there are no other synchronized or synchronize(this) in the class or sub-class or a synchronized (support) (instance wide locking) that could share the same monitor.

All three methods I removed synchronized from (classForName, create, nextSequenceValue) only touch context-local state, so the monitor was serializing every conversion thread for nothing. I rechecked the users and every other synchronized in the class: the rest lock a collection or a parameter rather than the Library instance, and there is no sub-class. The only two that do take that monitor are processClassVariables and processClassBuffers, and they are fine as they are: they reach the shared TRPL globals, the AST and the persisted tree, which is not in conflict with the context-local state the three methods touch, so nothing was relying on them being mutually exclusive.

Constantin managed this area in 8363g. Constantin, what do you think?

Constantin, please assess. IMHO, I am fine to remove the .intern calls as they (might) be slower than actual keys and equals, considering annotations are short(ish) and alphanumerical.

Otherwise, I am ok with the changes in 11317b and I would like to proceed with Testing.