mj part time coding - 3 months
Merge request reports
Activity
mentioned in commit fa05e876
My first session, which lasted between middle of February and middle of March is over. I was able to work with increased throughput at the beginning of March, as promised. I thank you for your support, as well as integrators and reviewers for their time.
Merged branches and patches
I looked around to find the current problems, that I’m able to solve and identified the following, starting from the completely solved ones:
Each reduced the recompilation time by about 10 times, or 30 minutes, so an hour worth of GitHub Actions’ time (which do cost some money)
I helped @moneromoo by patching his:
- easylogging++ memory corruption PR, where I added tests, which confirm the improvement and secure it against regressions
- His Triptych integration branch, with fixing compilation on MinGW, FreeBSD and OSX with this patch.
I helped partially to reduce the risk of crashes of the Mining Test by simply increasing the timeout of RX initialization. Later it turned out not to be that simple. More on this below in the list of newly open branches.
Newly opened branches
- Most notably, I took a serious approach towards fixing the random Mining Test crash. The full story can be found here. This should fix the problem once and for all, regardless of how overused the GitHub machines are at the time of testing. Such an investment however carried a huge time cost. A lot of time for this branch was taken up by the very constructive review by @iamamyth and addressing the review points. They all helped to iron the idea to almost a perfect shape, as well as spawned some further ideas.
- Related to the above, I prepared the
functional_tests_rpc
to be executed under Python3, instead of Python2. There are 2 main reasons: a) It’s becoming increasingly hard to operate in Python2, when more than simple functionality is requested, and b) Python2 is not supported anymore, not even with security patches, since the beginning of 2020 - Decreased every next execution of core_tests by 8 times or 50 minutes, using caching.
- I identified and fixed a new compilation warning, which is totally spamming the logs.
- I added the Epee headers into the project files, which is important when working with IDEs, which the CMake is able to generate projects for. This eases the continuation of Epee improvements.
- Epee can be linked dynamically after merging this. This reduces the dynamically compiled binaries by almost 10% and decreases the linkage time, when modifying Epee.
- I addressed a user’s issue, where the goal was to enable compilation on ARM Mac (in this case an SoC called “M1”). I believe, that supporting ARM based hardware is important for better adoption, since ARM CPUs are perfectly suited to run a full node, because of their low power consumption.
- Trying to clean up some potential memory bugs. Starting with Epee.
- Fortifying the source by treating some crucial warnings as compilation errors (effort against crashes and Undefined Behaviors)
- Decreased the recompilation time of the
ubuntu-test
job, using ccache and appropriate compilation flags
Documentation
- doc: reduced the Boost library list to the absolute minimum for Ubuntu, based on other work. This will help Monero package maintainers to cut on dependencies.
- doc: mentioned python requests functional tests' dependencies
I also took part in reviews of other contributors. I hope it was useful.
Edited by mjThank you for the detailed update, love seeing this kind of detail out if CCS contributors and goes to show how well-spent this community XMR is.
Keep up the great work, @mj!
Hi @mj - also thanks from me for the detailed report. Really interesting to read through, and a great example of time well spent.
Thank you, John. Apart from having a wild satisfaction from resolving the various bottlenecks of a project, where I'm heavily invested in, I also find it a bit sentimental to get such a reception (here and on Reddit), while getting paid for my work. Something, that my bosses were never able to think about.
Dear Community,
This time I focused on getting a large number of the already opened branches to be merged, with quite a success, since many of the most important ones were merged indeed. In a single sentence, the major merged branches improved stability and speed of the CI.
Before we dive into the PRs, in my external project I added recently LCov report (line coverage for testing). For those who don't know what it means, it helps in verifying if the code, that you produce, maintain or test, is actually being touched by the tests. Or in broader terms, whether the action that you perform (changing code), actually has the effect, that you expect it to have (be tested to be able to tell that it works as you wanted it). This is being generated for each of the series of merges made for then open PRs. After generating the report, I had 2 major findings:
- the base libraries (like Epee) are being unit tested rather poorly with only about 60% of coverage.
- EasyLogging++ library was not producing line coverage at all. After applying a patch mentioned further on, it turned out that only 52% of EL++ is being covered (if you're interested, please download and extract this with 7Zip).
Let's move on to the merged branches and their impact. Then to the newly opened ones as usual. This time some of them are already merged!
Merged branches
Major
- CI: Adaptive mining test : An agglomerate of functional tests was using an important, yet unstable mining test, which used to fail with about 40% probability. In such repetitive cases, humans get used to ignore the results of such a test. It would be only half that bad, if the failure only concerned the test in question (ie.: only mining), risking regressions in mining, but since this is an agglomerate, it shows the same error in logs as the final verdict, if any other small test fails. This means, that if people learn to ignore the whole test result, there is also a risk, that they ignore other tests of this agglomerate. Unfortunately this is exactly what happened... twice, before my PR got merged. I tried to save the day by applying a fix, meant to temporarily disable the then buggy feature, but after a day or so my fix was properly superseded with the Author's final fix, so early enough :). It took a good deal of time to maintain this branch, making it acceptable for all the reviewers, and setting up multiple test repetitions, in order to prove, that the PR fixes the issue, as I promised. If you're interested, you may see the results at the end of the PR.
- CI: Fix a random network test crash : As a fix of a random crash this belongs to the same category as the previous fix. The probability of crashing was about 10%. Lower, but still annoying and confusing, as one might think, that he/she was to be blamed for the crash with an unrelated own change. For this PR, I can attribute to myself detecting the issue by setting up repetitive tests, then communicating it on IRC, and reproducing the problem with the minimal time effort, so that it can reproduced locally within a fast loop. This part was done together with
wfaressuissia
, who later on provided the real fix and a exhaustive explanation. I also PR'd the fix to the latest release, not only master. - CI: ccache for ubuntu-test: Typical run of the ubuntu-test lasts 2 hours. With ccache, the time is reduced down to 1:25 h, so by about 25%. In order for this fix to work (for the binaries to be runnable on all x64 machines), I had to compile them as generic binaries (unlike before). The neat side effect is, that only now the test actually tests what is being distributed to the users, and not the locally optimized binaries for the machine they're compiled on. In most of the cases there's no difference between the generic and the optimized ones, other than a tiny bit faster execution time of the latter, but there is always a risk, that the optimized ones actually operate differently than the generic ones, and I don't mean just "slower", but really "differently". From now on the risk isn't pushed onto the Users anymore.
- Monero on ARM Mac: Extending the adoption a bit by allowing for Monero to be compiled on an ARM Mac. This PR addressed a User's issue.
Minor
- Epee: Added header files to project: This makes the IDE integration with Epee library possible.
- CI: Switched from Python2 to Python3 in tests: Python2 was not receiving any security updates anymore since the beginning of 2020.
- Compilation: Fixed a compilation warning for the whole project: This warning was spamming the logs big time. So much, that you couldn't see anymore what the logs were about.
- Compilation: Fixed a compilation warning in ut/levin.cpp: Another warning, but not that rough.
Documentation
- Added ccache to README.md as well as to Mac's Brewfile for an easier automated installation. That's really all you have to do to use ccache in Monero, because my first ever merged branch here handles this automatically everywhere.
- Reduced the Boost dependency list to only the currently required libraries. I mentioned it earlier already, but now it's merged.
Newly open branches
- Epee: Compression of the rotated logs: I'm trying to introduce zlib and its Boost wrapper across all the supported systems (which took the most of the time invested for this PR). Apart from the general benefits of introducing a fully portable compression library, this PR specifically tries to zip the rotated Monero logs, as a response to a User's issue about his logs being spammed, taking up 5 GB of disk space just to hold the produced spam. This PR doesn't really fix the cause of the User's issue, but if a similar spam issue was to be repeated, at least it would only take about 100 MB to store the spam, rather than 5 GB. Think of it as form of Defensive programming - adapting ahead of time to unforseen bugs of various nature in the future. The branch is technically ready to be merged, but many developers shared their mixed feelings, and it might need some time to reach a consensus here.
- Reproducible Builds: Automated the verification of binaries : All I did here was creating a GitHub Action, which calls the already written Python script and shows the results nicely in GitHub's GUI. It was a small, but useful thing, I think. Here you can see it in action (pun intended).
- Reproducible Builds: Added my PGP and hashes: I set up everything to verify the reproducibility of the latest version 0.17.2.0, and am all set for future versions. It was about time to finally add my PGP key.
- Reproducible Builds: Updated docs of Gitian: While trying to setup the RBs, I noted several inaccuracies in the documentation. With the help of my Reviewer: @hyc, we were able to sort it out and write some handy scripts to ease the process.
- Tests: Enabling test coverage for external repositories: It turns out, that the EasyLogging++ library couldn't be evaluated for the test coverage, since it the code coverage settings don't reach it, and if they did, the library would discard them anyway. By creating a reusable macro for setting the code coverage flags, I can force EasyLogging++ to produce code coverage report as well.
That's it for this month. Thank you for all kinds of support, big or small :)
Edited by mj
Dear Community,
The major themes (and oddities) of my last month's work would be:
- Easing the introduction for new Developers by documenting how to address common problems and work efficiently (Debugging, using IDEs and Test Driven Development)
- Many small things, which would otherwise lead to some dose of annoyance (compilation warnings, CMake changes)
- Confirming (across the time, that passed) the repeatable green builds for proper PRs instead of suffering from randomly failing ones. For me personally this has greatly improved my confidence and reduced stress level while waiting for the CI's verdicts. I associate this kind of stress with the negative feelings, that I experience during gambling. I contributed one more fix in this subject as well (also listed below).
- Doing some research on how much the distributed binaries could be reduced in terms of size. For instance, the Windows binaries could be 50% smaller.
- I extended my weekly Monero health report a bit, using more elaborate Clang Build (time) Analyser report. Thanks to this change, it became even more obvious where most of the compilation time is spent. Associated with this, I lifted the graph complexity restrictions in the Doxygen report. Unlike before, now even the most complex plots can be generated for closer investigation. Get your forks, spoons and Napoli sauce ready for this!.
- Re: 1), after noticing, that I have quite a few developers' utility branches still not merged, I decided, that I would store them in a patch form in a seperate repository and instruct the devs how to use them via the documentation file, that I created, since currently all this (IMHO) kick-ass improvements go to a waste. Of course care shall be taken to state explicitly, that neither the Monero Devs, nor the Maintainers shall take any responsibility for these patches, and that they serve as ad-hoc solutions. This is a WIP though.
- There were many new PRs, where I was able to help with my reviews, and didn't hesitate to comment/correct as they were arriving.
It is also worth mentioning, that my 3-month development period reaches its end. I found it fun to work for Monero and I hope you consider my help useful and not too expensive. With regard to raising XMR prices, until my next 3 months are financed and approved (it you wish so), I will continue to work "for free", as wanting to give something back for your trust, by maintaining my open branches and taking part in reviews. The thing that I won't be doing is opening new branches, unless they should have something to do strictly with an unpredictable breaks of CI, that I feel responsible for. Same goes of course with fixing any of the features, that I introduced myself. Another reason for not opening new branches, is that I want to give the Devs some breath from my spam, that they need to digest slowly :)
Here's the usual summary of merged and newly opened branches:
Merged branches
- CI: Adaptive mining test revisited: Based on @moneromoo 's feedback, I improved the adaptive mining test, as soon as the feedback was reported to me, and took the liberty of doing some small refactoring in the original code.
- Doc: Compiling, debugging and testing efficiently: This is the documentation, that I mentioned in point 1). This will be an evolving document. One feedback that I got from @selsta, was that IDEs such as Eclipse and MS VS should be included.
- CMake New macro - finding all headers: As the name suggests, the macro automatically finds all headers in a directory. Including them manually is very error prone. It is important for having a properly defined build dependencies, so that a developer doesn't need to call
make clean
and rebuild from scratch, just to be sure, that all the symbols are there, or all the recently changed inlined headers are compiled in. It also helps a lot while working with IDEs, which are now able to auto-save the changed files before the compilation (Re: point 1)). The additional beauty of this function is, that whenever any developer adds new headers to the project, they will be added to the dependency list automatically. This is actually an extract from code already confirmed to work, and placed in such manner, that it can be used across the whole project, including the external repositories. - CI: Actions' build.yaml uses variables instead of repetitions: Just a cleanup of some repetitions and actually the first introduction of variables into the project (AFAIK)
Newly opened branches
- Tests: Core tests speed up by reusing a variable: 10% faster generation of the test data achieved (about 3-7 minutes, depending on the machine), by reusing a variable, which was being needlessly computed too many times in an inner loop. Some refactoring was required, which had a positive side effect of decoupling a large function with too many parameters and reduced code duplication.
- CMake: Strip targets (optional):
- CMake: Strip installed executables POC: Would save 50% space for the unpacked Windows executables and 20% for GNU/Linux.
- CMake: Trezor temporary file path: I did some small cleanup of an annoying element, but my PR was superseded with an another, broader PR.
- CI: Relaxed retry conditions for dependency downloads: On rare occasions one of the sources of dependencies would deny access for our automated downloaders. In such case an alternative URL is being queried. If such alternative fails, the build crashes. By relaxing the retry conditions, I reduced the probability that both the original source and the alternative fail.
- Warnings: Unused variable in cryptonote_tx_utils.cpp
- Warnings: Unused variable in core/blockchain.cpp
- Warnings: Unused variable in core_tests:chaingen.h
- CI: Minimized boost dependencies: Done according to our official documentation, so that the CI verifies the docs and signals whenever there's a need to change them.
- CMake: Included missing headers of wallet2: Thanks to a recently merged branch, my
monero_find_all_headers()
macro, I was able already to identify and automatically include the missing header of wallet2 class. - CMake: Included missing headers of EasyLogging++: Similarly to above, one header wasn't being included.
- CMake: Adding inlined headers to the header list: Fixing an overlook from my side. The included file list was missing the ".inl" inlined file, which behave like headers. The currently used version however didn't introduce any regressions, as these .inl files were not being included manually anyway.
- Doc: New mining env variables in funtional_tests: I copy-pasted and extended the info about the new mining env vars from the Python code to the tests/README.md, so that the person, who runs the tests, doesn't need to dig into the code to find this info, as this is supposed to be a public API of the tests.
Compilation time reduction
This paragraph actually belongs to my second CCS proposal, which moves at its own slow pace, but I though I'd just mention here, that I'm still working on it for as often as the reviewers are able to check my already opened branches. In that other CSS I will write a bit longer report on the matter from the latest and a broader perspective, reinforced by the auto-generated reports, that weren't available previously. I will also want to discuss with you guys there how to fairly distribute the accumulated stash, in relation to how much work was possible to be done so far, since I don't feel like I'm being entitled to collect the whole sum for myself at the current XMR/USD prices.
The branches opened in that topic are:
- wallet2: abstraction POC: representing wallet2 as an abstract interface, like in the Java/C# way of thinking, so abstractions and forward declarations only. An overall compilation time reduction was about 2.5%, but the individual, most affected files, enjoyed speedups of up to 40%! A lot more is possible to be achieved if we continue this way.
- epee: moved storages/parserse_base_utils.h to .cpp: Just a small change, of moving code from .h to .cpp files, without much impact for now, but I want to keep my PRs small.
Thanks for reading and for your lasting support!
mj
One more thing worth mentioning is, that the deterministic (green) CI builds allowed us now to impose a "failed build --> no merge" policy, meaning, that we are much safer against regressions now. Also, the Reviewers are now able to skim the PR list, and immediately ignore the failed builds, as now they are an indication, that the given failed PR wasn't actually ready yet, but only an another CI machine/compiler was able to detect it (meaning, that the author wasn't aware of it).
mentioned in merge request !231 (merged)
mentioned in merge request !266 (merged)
mentioned in merge request !283 (closed)
mentioned in merge request !287 (merged)