Skip to content
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.

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

Replace SOURCE[] with dynamic array #186

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

icgmilk
Copy link

@icgmilk icgmilk commented Apr 4, 2025

This pull request replace SOURCE (in src/global.c) with a dynamically allocated array instead of a static string. This change improves memory flexibility.

Changes

  • Replaced char* SOURCE with a dynamically allocated array using malloc/free.
  • Updated related logic accordingly.

Performance Analysis

Before: /usr/bin/time -v ./out/shecc ./src/main.c

 Command being timed: "./out/shecc ./src/main.c"
        User time (seconds): 0.15
        System time (seconds): 0.21
        Percent of CPU this job got: 99%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.36
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 756224
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 189673
        Voluntary context switches: 1
        Involuntary context switches: 2
        Swaps: 0
        File system inputs: 0
        File system outputs: 464
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

After: /usr/bin/time -v ./out/shecc ./src/main.c

Command being timed: "./out/shecc ./src/main.c"
        User time (seconds): 0.16
        System time (seconds): 0.33
        Percent of CPU this job got: 100%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.50
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 759424
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 190433
        Voluntary context switches: 0
        Involuntary context switches: 2
        Swaps: 0
        File system inputs: 0
        File system outputs: 464
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

The dynamic array version used slightly more memory, likely due to the overhead introduced by dynamic memory allocation.

Summary by Bito

This pull request enhances memory flexibility by replacing the static char* SOURCE with a dynamically allocated array. It introduces a new structure for the dynamic array and updates the lexer and parser for compatibility. While there is a slight increase in memory usage, overall performance is improved.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 2

@jserv jserv requested review from fennecJ, DrXiao and ChAoSUnItY April 5, 2025 00:51
@@ -727,7 +784,7 @@ void global_init()
PH1_IR = malloc(MAX_IR_INSTR * sizeof(ph1_ir_t));
PH2_IR = malloc(MAX_IR_INSTR * sizeof(ph2_ir_t));
LABEL_LUT = malloc(MAX_LABEL * sizeof(label_lut_t));
SOURCE = malloc(MAX_SOURCE);
SOURCE = create_source(MAX_SOURCE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking of choosing an appropriate power of 2 for MAX_SOURCE. perhaps 2^18 (262144 B, or 256 KiB) or 2^19 (524288 B, or 512 KiB) would be better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then, we do need scientific ways to calculate.

This commit replace `SOURCE` (in `src/global.c`) with a dynamically allocated array instead of a static string. Including:
- Replaced char* SOURCE with a dynamically allocated array using malloc and free.
- Updated related logic accordingly.

Co-authored-by: Kyle Lin <[email protected]>
@@ -270,6 +270,14 @@ typedef struct {

typedef struct basic_block basic_block_t;

/* source dynamic array definition*/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this blank line and refine the comments.

return array;
}

bool source_expand(source_t *self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename self to src for readability.


void source_release(source_t *self)
{
if (self) {
Copy link
Collaborator

@jserv jserv Apr 7, 2025

Choose a reason for hiding this comment

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

Rewrite as following:

    if (!src)
        return;

    free(src->elements);
    free(src);

@jserv jserv changed the title Replace char* SOURCE with dynamic array Replace SOURCE[] with dynamic array Apr 7, 2025
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Read https://cbea.ms/git-commit/ carefully and enforce the rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants