Skip to content

perf(ini): Remove unnecessary strlen call in INI::readLine#2253

Open
Caball009 wants to merge 2 commits intoTheSuperHackers:mainfrom
Caball009:optimize_ini_readLine
Open

perf(ini): Remove unnecessary strlen call in INI::readLine#2253
Caball009 wants to merge 2 commits intoTheSuperHackers:mainfrom
Caball009:optimize_ini_readLine

Conversation

@Caball009
Copy link

@Caball009 Caball009 commented Feb 3, 2026

This PR removes an unnecessary call to strlen in INI::readLine that was used for the CRC computation. The line length can be tracked while parsing the line.

@Caball009 Caball009 added Minor Severity: Minor < Major < Critical < Blocker Performance Is a performance concern labels Feb 3, 2026
@Caball009 Caball009 changed the title perf(ini): Remove unnecessary strlen call for CRC computation perf(ini): Remove unnecessary strlen call in INI::readLine Feb 3, 2026
@greptile-apps
Copy link

greptile-apps bot commented Feb 3, 2026

Greptile Overview

Greptile Summary

This PR optimizes INI::readLine by eliminating an unnecessary strlen call during CRC computation. The line length is now tracked during parsing using pointer arithmetic (p - m_buffer), which is O(1) instead of O(n). The implementation correctly handles all cases including lines with comments (semicolons), empty lines, and EOF.

Changes:

  • Added lineLength variable to track parsed line length
  • Special handling for semicolon comments: saves length before comment, continues reading to consume comment characters
  • Empty lines (starting with semicolon) use ~0u marker, converted to 0 after parsing
  • Assertion validates computed length matches strlen in debug builds
  • Performance improvement: removes O(n) string scan that was redundant with parsing work

Issue found:

  • Format specifier bug: uses %u for size_t instead of %zu (line 544)

Confidence Score: 4/5

  • Safe to merge after fixing the format specifier bug
  • The optimization is sound and correctly implements length tracking across all code paths. The logic handles normal lines, comments, empty lines, and EOF correctly. One minor syntax issue with format specifier needs correction before merge.
  • No files require special attention beyond the format specifier fix

Important Files Changed

Filename Overview
Core/GameEngine/Source/Common/INI/INI.cpp Optimizes INI line reading by eliminating strlen call; has format specifier bug that needs fixing

Sequence Diagram

sequenceDiagram
    participant Caller
    participant ReadLine as INI readLine
    participant Buffer as m_buffer
    participant Xfer as s_xfer
    
    Caller->>ReadLine: Read next line
    activate ReadLine
    
    ReadLine->>ReadLine: Initialize lineLength = 0
    
    alt End of file
        ReadLine->>Buffer: Set to empty string
    else Normal reading
        loop Until newline or buffer full
            ReadLine->>Buffer: Read character from file
            
            alt Character is semicolon
                ReadLine->>Buffer: Terminate string at semicolon
                ReadLine->>ReadLine: Save lineLength before semicolon
                ReadLine->>ReadLine: Continue reading comment
            else Character is newline
                ReadLine->>Buffer: Terminate string
                ReadLine->>ReadLine: Break loop
            else Character is whitespace
                ReadLine->>Buffer: Convert to space
            end
        end
        
        ReadLine->>Buffer: Final null termination
        
        alt lineLength not yet set
            ReadLine->>ReadLine: Calculate lineLength from pointer
        else lineLength is empty line marker
            ReadLine->>ReadLine: Set lineLength = 0
        end
    end
    
    alt CRC transfer enabled
        ReadLine->>ReadLine: Assert lineLength equals strlen
        ReadLine->>Xfer: Transfer buffer with computed length
        Note over Xfer: CRC computed without strlen call
    end
    
    ReadLine-->>Caller: Line ready
    deactivate ReadLine
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

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

This adds a lot of extra complexity that is more prone to errors than the strlen was. Is it worth it? Can it be simplified?

@Caball009
Copy link
Author

Caball009 commented Feb 4, 2026

I think I feel the same way about it as you do. The first iteration of this PR was much simpler but ignored the comment logic for ;
I'm going to take a look if there's something to simplify.

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

Labels

Minor Severity: Minor < Major < Critical < Blocker Performance Is a performance concern

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants