Optimize with ossl un likely coveralls approach by jogme · Pull Request #27961 · openssl/openssl · GitHub | Latest TMZ Celebrity News & Gossip | Watch TMZ Live
Skip to content

Optimize with ossl un likely coveralls approach #27961

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Optimize with ossl un likely coveralls approach #27961

wants to merge 9 commits into from
+73 −65

Conversation

jogme
Copy link
Contributor

@jogme jogme commented Jul 3, 2025 •
edited
Loading

Use ossl_(un)likely to optimize branch usage.

Fixes: openssl/project#1233

I focused on the hot-paths in this PR where the hits were above 100 000 000. These lines were identified by coverage reports on running the test suite. Then I wrote a script to get the lines from the coverage file. The script is:

# Let's write a script which takes the output of generated coverall data and
# analyzes the if-else branches for how many times where they taken and generate
# an output with the files and lines of focus.

#Preparation for this script to be runnable:
# $ ./config -fprofile-arcs -ftest-coverage
# $ make clean && make -j$(nproc) && make -j$(nproc) test
# $ gcovr --json --txt-metric=branch --json-pretty --decisions -o coverage.json
# modify the openssl_dir variable below to match your system
# run this script

import json
import re

name = 'coverage.json'
above_limit = 100000000
below_limit = 100000
openssl_dir = ''
out = []

with open(name) as f:
    data = json.load(f)

for x in data['files']:
    file_name = x['file']
    out.append(f'{file_name}')
    for l in x['lines']:
        if len(l['branches']) != 2 or l['count'] < above_limit:
            continue
        with open(openssl_dir+file_name) as f:
            source_file = f.read().split('\n')[:-1]
            line = source_file[l['line_number']-1]
            if re.search(r'\b(for|while)\b', line):
                continue
        if_branch = 0
        else_branch = 0
        for b in l['branches']:
            if b['fallthrough'] == True:
                if_branch = b['count']
            else:
                else_branch = b['count']
        try:
            function_name = l['function_name']
        except KeyError:
            function_name = 'None'
        big = if_branch if if_branch > else_branch else else_branch
        small = if_branch if if_branch <= else_branch else else_branch
        # some corner cases and
        # optimize only when the smaller is quite a small number
        if big < above_limit or small > below_limit:
            continue
        out.append(f'{l["line_number"]+1:>4}: {line:<75}// {if_branch}/{else_branch}')
    # nothing found in the file
    if out[-1] == file_name:
        del out[-1]

for x in out:
    print(x)
Checklist
  • documentation is added or updated
  • tests are added or updated

jogme added 4 commits July 3, 2025 12:04
@jogme
crypto/evp/digest.c: optimize ossl_(un)likely
c95153f 
Signed-off-by: Norbert Pocs <norbertp@openssl.org>
@jogme
crypto/bn/bn_lib.c: optimize - seems to bring not much benefit
df309fc 
Signed-off-by: Norbert Pocs <norbertp@openssl.org>
@jogme
include/crypto/md32_common.h: optimize ossl_(un)likely
04ad0d1 
Signed-off-by: Norbert Pocs <norbertp@openssl.org>
@jogme
crypto/bn/bn_gf2m.c: optimize ossl_(un)likely
4107370 
Signed-off-by: Norbert Pocs <norbertp@openssl.org>
@jogme jogme force-pushed the optimize_with_ossl_un_likely_coveralls_approach branch from d7a9337 to b8c7f60 Compare July 3, 2025 14:55
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jul 3, 2025
@slontis
Copy link
Member

slontis commented Jul 4, 2025

So if we know every call to a function is not likely e.g. ossl_prov_is_running(). Is the proposal to do this check everywhere?
In most places it may not make much difference, except where it gets called multiple times inside an algorithm (e.g. inside PBKDF2 which has an large iteration count).

paulidale
paulidale reviewed Jul 4, 2025
View reviewed changes
paulidale
paulidale reviewed Jul 4, 2025
View reviewed changes
crypto/params.c
@@ -868,7 +869,7 @@ int OSSL_PARAM_get_uint64(const OSSL_PARAM *p, uint64_t *val)
return 0;
}

if (p->data_type == OSSL_PARAM_UNSIGNED_INTEGER) {
if (ossl_likely(p->data_type == OSSL_PARAM_UNSIGNED_INTEGER)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of similar ones in this file that aren't updated. I'd suggest staying consistent and doing all or none of these checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, but the data says different. (Probably will need to updated the description and add more info, but for now tl;dr:) I checked the coveralls report, to check for hot-paths (depending on our test suite) and focused only on the most used lines. If you check this file, no other places have that huge usage as this one. I'm focusing only on the most used places where this optimization can really matter.

Copy link
Contributor

@paulidale paulidale Jul 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure our CI tests are all that representative or normal use. They will be for this function (asking for it's primary type) but not more generally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the test suite is not completely showing true usage, but it could show at least some data. It is very hard to get high amount of real data on this.

jogme added 5 commits July 4, 2025 09:47
@jogme
crypto/mem.c optimize ossl_(un)likely
eaf9d10 
Signed-off-by: Norbert Pocs <norbertp@openssl.org>
@jogme
crypto/threads_pthread.c optimize ossl_(un)likely
b7a748d 
Signed-off-by: Norbert Pocs <norbertp@openssl.org>
@jogme
crypto/init.c optimize ossl_(un)likely
552a34a 
Signed-off-by: Norbert Pocs <norbertp@openssl.org>
@jogme
crypto/params.c optimize ossl_(un)likely
e0c88da 
Signed-off-by: Norbert Pocs <norbertp@openssl.org>
@jogme
providers/implementations/digests/sha3_prov.c optimize ossl_(un)likely
0dbd7f9 
Signed-off-by: Norbert Pocs <norbertp@openssl.org>
@jogme jogme force-pushed the optimize_with_ossl_un_likely_coveralls_approach branch from b8c7f60 to 0dbd7f9 Compare July 4, 2025 07:59
@jogme
Copy link
Contributor Author

jogme commented Jul 4, 2025

Updated the description. The output I get from the script locally on this branch is:

crypto/bn/bn_gf2m.c
 348:         if (ossl_likely(d0))                                               // 0/108964314
crypto/engine/eng_init.c
 111:     if (e == NULL)                                                         // 47104/118129328
crypto/engine/eng_table.c
 213:     if (!(*table)) {                                                       // 112728314/40525
crypto/evp/digest.c
  33:         if (ossl_unlikely(ctx->digest->cleanup != NULL)                    // 1/126571782
 166:     if (ossl_unlikely(ctx->pctx != NULL)                                   // 46/109520102
 190:         if (ossl_unlikely(ctx->digest == NULL)) {                          // 20/103191136
 205:     if (ossl_unlikely(ctx->engine != NULL)                                 // 868/109519220
 218:     if (ossl_likely(impl == NULL))                                         // 0/109519220
 228:             || ctx->engine != NULL                                         // 109519220/0
 230:             || tmpimpl != NULL                                             // 109505500/13720
 233:             || (ctx->flags & EVP_MD_CTX_FLAG_NO_INIT) != 0                 // 109505500/0
 234:             || (type != NULL && type->origin == EVP_ORIG_METH)             // 0/109505499
 251:         if (ossl_unlikely(!ossl_assert(type->prov != NULL))) {             // 0/108088150
 298:     if (ossl_unlikely(ctx->digest->dinit == NULL)) {                       // 0/109505473
 388:     if (ossl_unlikely(count == 0))                                         // 392906228/9613
 391:     if (ossl_unlikely((ctx->flags & EVP_MD_CTX_FLAG_FINALISED) != 0)) {    // 1/392906227
 396:     if (ossl_unlikely(ctx->pctx != NULL)                                   // 112/392906115
 417:     if (ctx->digest == NULL                                                // 392906119/2
 418:             || ctx->digest->prov == NULL                                   // 392870279/35840
 419:             || ossl_unlikely((ctx->flags & EVP_MD_CTX_FLAG_NO_INIT) != 0)) // 0/392870279
 422:     if (ossl_unlikely(ctx->digest->dupdate == NULL)) {                     // 0/392870279
crypto/init.c
 506:     if (ossl_unlikely(stopped)) {                                          // 3/145479230
 525:     if (ossl_likely(CRYPTO_atomic_load(&optsdone, &tmp, NULL))) {          // 145479230/0
crypto/mem.c
 192:     if (malloc_impl != CRYPTO_malloc) {                                    // 97/140410701
 199:     if (ossl_unlikely(num == 0))                                           // 1186/140409515
 203:     if (allow_customize) {                                                 // 6823/140402692
 213:     if (ossl_likely(ptr != NULL))                                          // 0/140409515
 349:     if (free_impl != CRYPTO_free) {                                        // 13/211696267
crypto/threads_pthread.c
 326:         if (data->thread_qps[i].lock == lock) {                            // 0/154178310
 686:     if (ossl_unlikely(pthread_once(once, init) != 0))                      // 0/120741990
include/crypto/md32_common.h
 166:     if (ossl_unlikely(len == 0))                                           // 257480905/13
 170:     if (ossl_unlikely(l < c->Nl))              /* overflow */              // 0/257480905
providers/implementations/digests/sha3_prov.c
  82:     if (ossl_unlikely(len == 0))                                           // 120072472/0

where the numbers at the end should be the if and else taken - but that's not correct. Still it gives a good idea and with some lookup in the coverage.json file, it is identifiable what's going on.

@jogme
Copy link
Contributor Author

jogme commented Jul 4, 2025

So if we know every call to a function is not likely e.g. ossl_prov_is_running(). Is the proposal to do this check everywhere? In most places it may not make much difference, except where it gets called multiple times inside an algorithm (e.g. inside PBKDF2 which has an large iteration count).

I updated the description saying how was this outcome identified. We don't want to put this check all over the code, rather focusing on critical places

@jogme
Copy link
Contributor Author

jogme commented Jul 4, 2025

I still need to find a way to properly benchmark this; so leaving it in draft for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulidale paulidale paulidale left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
severity: fips change The pull request changes FIPS provider sources
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Start applying ossl_likely/ossl_unlikely macros to optimize code paths
3 participants
@jogme @slontis @paulidale

TMZ Celebrity News – Breaking Stories, Videos & Gossip

Looking for the latest TMZ celebrity news? You've come to the right place. From shocking Hollywood scandals to exclusive videos, TMZ delivers it all in real time.

Whether it’s a red carpet slip-up, a viral paparazzi moment, or a legal drama involving your favorite stars, TMZ news is always first to break the story. Stay in the loop with daily updates, insider tips, and jaw-dropping photos.

🎥 Watch TMZ Live

TMZ Live brings you daily celebrity news and interviews straight from the TMZ newsroom. Don’t miss a beat—watch now and see what’s trending in Hollywood.