Laura Montemayor (425414b4) at 17 Mar 18:50

... and 1957 more commits

@rkadam3 - this is looking good - I've got 2 suggestions for you!

Could you please add the experimental marker to this and the other fields down below? This gives us flexibility to adjust the schema based on real usage feedback without creating breaking changes. This is especially important for new features where the API surface might need refinement

:thinking:This builds the semver arrays with || 0 duplication - we could extract to a helper, like so:

def to_semver_array(version)
  [version.semver_major || 0, version.semver_minor || 0, version.semver_patch || 0]
end

def older_than_latest?(version)
    return false unless version && latest_version

  to_semver_array(version) <=> to_semver_array(latest_version) == -1
end

Thanks @rkadam3 - I will take a look. In the meanwhile you should get a docs review from @marcel.amirault as well

@rkadam3 - we can't change public facing fields unless they are marked as experimental:. Otherwise it is a breaking change.

Thanks for putting it together - I will review the other MR shortly

I think we should add the > [!warning] section @manuelgrabowski mentioned about string truthiness. Based on @furkanayhan 's comment and example maybe something like this?

@tarunraja Thanks for working on this! The code looks good - I just have some questions re the scope.

The issue description says this fixes the case when using $CI_JOB_TOKEN with the trigger pipeline API, but the implementation allows TRIGGER_PAYLOAD for all pipelines where source = :pipeline, not just those using CI_JOB_TOKEN.

Can you confirm

1. What are all the scenarios where source = :pipeline can occur?
   1. If there are any - is TRIGGER_PAYLOAD always GitLab-generated?
   2. Is there any scenario where allowing TRIGGER_PAYLOAD for source = :pipeline could allow a user to bypass the permission check in an unintended way?

I mention this because the original issue #557381 (closed) mentions that TRIGGER_PAYLOAD is "defined by GitLab when triggering webhooks", which means it's GitLab-controlled. I want to make sure we're not inadvertently allowing user-provided TRIGGER_PAYLOAD values in other :pipeline scenarios. 😬

If you can confirm that source = :pipeline only occurs when using the trigger Api, then this looks good to me!

Laura Montemayor (bdafad95) at 17 Mar 16:40

@egrieff Ah, that makes total sense - without the preloading, CachedCommit#to_hash ends up triggering N+1 queries for all those delegated fields. 🤔

However, I found a possible solution to the problem affecting this worker: use an existing SQL query that fetches the commit SHAs and build the CommitRequest objects from them (I created !227632 to try this).

This looks like a good solution - I'm happy to review this. Send it over my way when it's ready! (seems like it is?)

Laura Montemayor (5420b2ed) at 17 Mar 01:34

Laura Montemayor (52b4912d) at 17 Mar 01:33

... and 1 more commit

What does this MR do and why?

Optimizes Gitlab::Ci::Variables::Collection::Item.fabricate by avoiding unnecessary symbolize_keys calls when hash keys are already symbols.

symbolize_keys is called on every variable hash passed to Item.fabricate, which is one of the hottest paths during pipeline creation. In practice, the vast majority of callers already pass hashes with symbol keys, so symbolize_keys creates a redundant copy of the hash each time. This MR introduces fast_symbolize_keys which checks if keys are already symbols and skips the allocation in that case.

When string keys are detected, we log the occurrence to verify our assumption. After monitoring production, if no string keys are found, we can simplify the code further.

Plan

The plan is the fast iteration; deploy->enable->track->remove.

Then, do the same for dup allocation we discussed in !225902.

References

• Related to #512876
• Previous POC: !225902

Memory profiling results

# RAILS_PROFILE=true GITALY_DISABLE_REQUEST_LIMITS=true rails console

ActiveRecord::Base.logger = nil
@project = Project.find_by_full_path('gitlab-org/gitlab-clone')
@user = @project.first_owner
@merge_request = MergeRequest.find(153)
@merge_request_params = { allow_duplicate: true }

require 'memory_profiler'

def run
  Gitlab::SafeRequestStore.ensure_request_store do
    pipeline = ::MergeRequests::CreatePipelineService.new(
      project: @project, current_user: @user, params: @merge_request_params
    ).execute(@merge_request).payload

    raise "stop!" unless pipeline.created_successfully?
  end
end

run

report = MemoryProfiler.report do
  run
end

2.times do
  Gitlab::SafeRequestStore.ensure_request_store do
    ::Ci::DestroyPipelineService.new(@project, @user).execute(Ci::Pipeline.last)
  end
end

io = StringIO.new
report.pretty_print(io, detailed_report: true, scale_bytes: true, normalize_paths: true)
puts io.string

Overview

Key improvement: symbolize_keys elimination

activesupport/core_ext/hash/keys.rb allocated memory dropped from 68.68 MB → 12.40 MB (-56.28 MB, -82%). This is the direct result of skipping redundant hash duplication.

Allocated memory by top gems

Allocated memory by top files

Allocated memory by top classes

Note on feature flag overhead

With the feature flag enabled, total allocation is 1.42 GB — ~30 MB more than the FF-removed version (1.39 GB). This overhead comes from the FF check itself and the hash.first call creating a temporary [key, value] array per invocation (~28 MB in extra Array allocations). This disappears once the FF is removed.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Laura Montemayor (cc597289) at 17 Mar 00:53

Yup, updating!

hi @furkanayhan and @avielle 👋 here's the MR for expanding (some) variables lazily - wdyt? Note - this doesn't expand ALL variables (although it covers almost 90% of the most frequently used ones) - see the description up above for more details.

Laura Montemayor (c63a8f2a) at 17 Mar 00:23

... and 775 more commits

Laura Montemayor (ba9c676c) at 17 Mar 00:01

@furkanayhan great! Let's merge this.

@furkanayhan will one entry be enough for data analysis though?