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

Append in place for strings and arrays [failure unrelated] #7999

Merged
merged 1 commit into from Jan 22, 2015

Conversation

7 participants
@cirosantilli
Contributor

cirosantilli commented Oct 9, 2014

It's faster.

Behavior should be unchanged: no function inputs were altered, only local variables.

Benchmark:

Benchmark.bm(first_col_width) do |benchmark|
  benchmark.report('+= [0]') do
    a = []
    N.times { a += [0] }
  end

  benchmark.report('<< 0') do
    a = []
    N.times { a << 0 }
  end

  benchmark.report('<< [0, 1]') do
    a = []
    N.times { a += [0, 1] }
  end

  benchmark.report('push(*[0, 1])') do
    a = []
    N.times { a.push(*[0, 1]) }
  end

  benchmark.report('+= "ab"') do
    a = ''
    N.times { a += 'ab' }
  end

  benchmark.report('<< "ab"') do
    a = []
    N.times { a << 'ab' }
  end
end

Result:

                    user     system      total        real
+= [0]             0.090000   0.010000   0.100000 (  0.093640)
<< 0               0.000000   0.000000   0.000000 (  0.000706)
<< [0, 1]          0.170000   0.000000   0.170000 (  0.164410)
push(*[0, 1])      0.000000   0.000000   0.000000 (  0.001339)
+= "ab"            0.020000   0.000000   0.020000 (  0.024198)
<< "ab"            0.000000   0.000000   0.000000 (  0.001242)
@TeatroIO

This comment has been minimized.

TeatroIO commented Oct 9, 2014

I've prepared a stage. Click to open.

@@ -417,7 +417,7 @@ def update_merge_requests(oldrev, newrev, ref, user)
# Update code for merge requests into project between project branches
mrs = self.merge_requests.opened.by_branch(branch_name).to_a
# Update code for merge requests between project and project fork
mrs += self.fork_merge_requests.opened.by_branch(branch_name).to_a
mrs.push(*self.fork_merge_requests.opened.by_branch(branch_name).to_a)

This comment has been minimized.

@houndci-bot

houndci-bot Oct 9, 2014

Redundant self detected.

@@ -431,8 +431,10 @@ def update_merge_requests(oldrev, newrev, ref, user)
end
def comment_mr_with_commits(branch_name, commits, user)
mrs = self.origin_merge_requests.opened.where(source_branch: branch_name).to_a
mrs += self.fork_merge_requests.opened.where(source_branch: branch_name).to_a
mrs = self.origin_merge_requests.opened.

This comment has been minimized.

@houndci-bot

houndci-bot Oct 9, 2014

Redundant self detected.

mrs += self.fork_merge_requests.opened.where(source_branch: branch_name).to_a
mrs = self.origin_merge_requests.opened.
where(source_branch: branch_name).to_a
mrs.push(*self.fork_merge_requests.opened.

This comment has been minimized.

@houndci-bot

houndci-bot Oct 9, 2014

Redundant self detected.

@cirosantilli cirosantilli force-pushed the cirosantilli:append-inplace branch 2 times, most recently from dd41553 to 507beb7 Oct 9, 2014

@cirosantilli cirosantilli changed the title from [WIP] Append in place for strings and arrays to Append in place for strings and arrays Oct 13, 2014

contents += tree.trees
contents += tree.blobs
contents += tree.submodules
contents.push(*tree.trees)

This comment has been minimized.

@jvanbaarsen

jvanbaarsen Dec 23, 2014

Contributor

Would it be possible to write it as:
contents << tree.trees

That way its more easy to understand.

This comment has been minimized.

@dblessing

This comment has been minimized.

Member

dblessing commented Dec 26, 2014

@cirosantilli Can you please rebase?

@cirosantilli cirosantilli force-pushed the cirosantilli:append-inplace branch from 507beb7 to 3c973f4 Dec 29, 2014

@@ -10,13 +10,16 @@ def render_tree(tree)
tree = ""
# Render folders if we have any
tree += render partial: 'projects/tree/tree_item', collection: folders, locals: {type: 'folder'} if folders.present?
tree << render(partial: 'projects/tree/tree_item', collection: folders,
locals: {type: 'folder'}) if folders.present?

This comment has been minimized.

@houndci-bot

houndci-bot Dec 29, 2014

Space inside { missing.
Space inside } missing.

# Render files if we have any
tree += render partial: 'projects/tree/blob_item', collection: files, locals: {type: 'file'} if files.present?
tree << render(partial: 'projects/tree/blob_item', collection: files,
locals: {type: 'file'}) if files.present?

This comment has been minimized.

@houndci-bot

houndci-bot Dec 29, 2014

Space inside { missing.
Space inside } missing.

@cirosantilli cirosantilli force-pushed the cirosantilli:append-inplace branch from 3c973f4 to 6953b29 Dec 29, 2014

@cirosantilli cirosantilli changed the title from Append in place for strings and arrays to [WIP]Append in place for strings and arrays Dec 29, 2014

@cirosantilli cirosantilli changed the title from [WIP]Append in place for strings and arrays to [WIP] Append in place for strings and arrays Dec 29, 2014

@cirosantilli cirosantilli force-pushed the cirosantilli:append-inplace branch from 6953b29 to 1c1b0a1 Dec 30, 2014

@cirosantilli cirosantilli changed the title from [WIP] Append in place for strings and arrays to Append in place for strings and arrays Dec 30, 2014

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Dec 30, 2014

@dblessing updated.

@dblessing

This comment has been minimized.

Member

dblessing commented Dec 30, 2014

Thanks!

@cirosantilli cirosantilli changed the title from Append in place for strings and arrays to Append in place for strings and arrays [failure unrelated] Dec 30, 2014

@cirosantilli cirosantilli force-pushed the cirosantilli:append-inplace branch from 1c1b0a1 to 33c9f05 Jan 1, 2015

@jvanbaarsen

This comment has been minimized.

Contributor

jvanbaarsen commented Jan 4, 2015

@cirosantilli Retriggered the build, we cant merge PR's when red, sorry!

@dblessing

This comment has been minimized.

Member

dblessing commented Jan 17, 2015

Tests were passing prior to adding the 'Ready for Merge' label so hopefully they still pass after

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Jan 17, 2015

@dblessing ah, labelling retriggers tests?

@dblessing

This comment has been minimized.

Member

dblessing commented Jan 17, 2015

Yes, it really makes no sense and is annoying. I'm not sure why.

@Razer6

This comment has been minimized.

Member

Razer6 commented Jan 18, 2015

@cirosantilli @dblessing Semaphore already knows about this issue.

@jvanbaarsen jvanbaarsen added this to the 7.8 milestone Jan 18, 2015

dzaporozhets added a commit that referenced this pull request Jan 22, 2015

Merge pull request #7999 from cirosantilli/append-inplace
Append in place for strings and arrays [failure unrelated]

@dzaporozhets dzaporozhets merged commit 2b10520 into gitlabhq:master Jan 22, 2015

2 checks passed

default Hound has reviewed the changes.
semaphoreci The build passed on Semaphore.
Details

@cirosantilli cirosantilli deleted the cirosantilli:append-inplace branch Jan 22, 2015

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