[SERVER-55246] Add SCons support for TCMalloc tests and validate they pass Created: 17/Mar/21  Updated: 06/Feb/24  Resolved: 06/Feb/24

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major - P3
Reporter: Tammy Bailey (Inactive) Assignee: Backlog - Service Architecture
Resolution: Duplicate Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File test_run.log    
Issue Links:
Duplicate
duplicates SERVER-85737 Bring the TCMalloc integration up to ... In Progress
Assigned Teams:
Service Arch
Participants:
Story Points: 8

 Description   

In SERVER-54747 we created a build with the new TCMalloc library but the tests were not included as part of the build. The following items should be completed as part of this ticket:

  • The TCMalloc testing directory contains BUILD.bazel files for use with the Bazel build system; these Bazel files should be removed and the appropriate SCons files added, and the higher level SCons files updated as needed.
  • Verify the tests compile using the MongoDB toolchain version of gcc.
  • Verify the tests pass. If they don't pass, investigate as to why and update this ticket with the findings.


 Comments   
Comment by Alison Felizzi (Inactive) [ 30/Mar/21 ]

Hi acm , sorry about the confusion! I misinterpreted the meaning of a 'test registration' framework. I wasn't aware of the RegisterTest functionality in SCons (rather I incorrectly zeroed in on linking with mongo/unittest/unittest_main).

What you have suggested is much cleaner! I've updated my branch with your suggestion. Thanks for the feedback

Comment by Andrew Morrow (Inactive) [ 30/Mar/21 ]

Hi alison.felizzi - To be clear, my suggestion applied only to the SCons integration - not to switching tcmalloc away from using googletest. In some ways I wish that the mongo test framework really was googletest, rather than our hand rolled one. I agree with your decision not to alter the tcmalloc sources and to continue to use their test framework. However, I do feel pretty strongly that you would benefit from not re-implementing the existing in-build-system test execution facility as you have here: https://github.com/alisonfel/mongo/commit/4d34383be26b6881006c9f37599657ffe6ecac10#diff-197a920b4b71a216c0b726def0d4cc3d8cd1ce6f76b74e23a0374a3adce555f5R231

I think you could entirely make use of the facilities offered by the current build system to achieve this. There is no requirement that tests use the mongo test framework in order to participate in the build system level integrations. For instance the visibility tests: https://github.com/mongodb/mongo/blob/master/src/mongo/platform/SConscript#L89-L108

I think you could replace your existing create_tcmalloc_test with something like the following:

def create_tcmalloc_test(env, target, source, **kwargs):
    tc_test = env.Program(
        target=target,
        source=source,
        AIB_COMPONENT='tcmalloc',
        **kwargs
    )
    RegisterTest("$UNITTEST_LIST", tc_test[0])
    return tc_test
 
test_env.AddMethod(create_tcmalloc_test, "TCMallocTest")

That would get you the + syntax for these tests automatically without needing to roll your own Command implementation, and would mean that your tests were automatically included among the unit tests on evergreen. And when using -build-tools=next you would get the prove-tcmalloc-test alias. If you wanted to, you could keep your tcmalloc-testsuite-exec alias for now so that you weren't reliant on -build-tools=next until we do the next promotion. The tcmalloc-testsuite alias would no longer be needed as the AIB_COMPONENT=tcmalloc would mean that an install-tcmalloc-test alias would be automatically produced for you.

Comment by Alison Felizzi (Inactive) [ 30/Mar/21 ]

Hey acm, thanks for the useful feedback and raising some great points! Its clear that it would be more ideal to use the existing test registration and execution facilities (over using googletest for test registration and the custom Alias scheme for execution).

I took the aforementioned path as to avoid updating and patching the tcmalloc source. This being the path of least friction to mainly validate that the tests run and pass using the MongoDB toolchain and it's specific version of gcc 8.2 (being the main objective of this ticket). Also I was partially hesitant making significant tcmalloc source changes until the initial perf validations are completed.

Beyond this initial version of compiling and running the tests, would using the existing test framework provide additional validation/insight to this ticket? Would be more than happy to iterate using our test framework if so!

I believe the intention was that once the initial performance evaluation is completed it would be the point we start ensuring full/complete TCMalloc support throughout the codebase (Stage 3 of the Technical Design Doc). That would definitely be an ideal point to add complete integration with our server testing.

Comment by Andrew Morrow (Inactive) [ 29/Mar/21 ]

alison.felizzi - I think it would be somewhat better if you integrated with the existing test registration and execution alias facilities that apply to the other unit tests in the server tree, rather than rolling your own execution framework and Alias scheme. That way, as we make improvements to the test execution facilities the tcmalloc tests will benefit as well. Please reach out on #server-build-help if you would like some thoughts on how to go about that. Another benefit is that if for some reason it isn't currently possible to do so, we can make improvements to the test execution facilities to make it possible.

Generated at Thu Feb 08 05:35:55 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.