Skip to content

avoid unnecessary NewCreator calls#4529

Merged
estesp merged 1 commit intocontainerd:masterfrom
gongguan:creator
Sep 21, 2020
Merged

avoid unnecessary NewCreator calls#4529
estesp merged 1 commit intocontainerd:masterfrom
gongguan:creator

Conversation

@gongguan
Copy link
Copy Markdown
Contributor

@gongguan gongguan commented Sep 4, 2020 •
edited
Loading

Signed-off-by: Guanjun Gong [email protected]

containerd/cmd/ctr/commands/tasks/tasks_unix.go

Lines 75 to 99 in f543f2f

stdio := cio.NewCreator(append([]cio.Opt{cio.WithStreams(stdinC, os.Stdout, os.Stderr)}, ioOpts...)...)
if checkpoint != "" {
im, err := client.GetImage(ctx, checkpoint)
if err != nil {
return nil, err
}
opts = append(opts, containerd.WithTaskCheckpoint(im))
}
ioCreator := stdio
if con != nil {
ioCreator = cio.NewCreator(append([]cio.Opt{cio.WithStreams(con, con, nil), cio.WithTerminal}, ioOpts...)...)
}
if nullIO {
if con != nil {
return nil, errors.New("tty and null-io cannot be used together")
}
ioCreator = cio.NullIO
}
if logURI != "" {
u, err := url.Parse(logURI)
if err != nil {
return nil, err
}
ioCreator = cio.LogURI(u)
}

ioCreator can be built at most 4 times, we should avoid unnecessary init.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Sep 4, 2020

Build succeeded.

mikebrow
mikebrow reviewed Sep 4, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

see comment

Comment thread cmd/ctr/commands/tasks/tasks_unix.go Outdated
Copy link
Copy Markdown
Member

@mikebrow mikebrow Sep 4, 2020

Choose a reason for hiding this comment

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

suggest moving this if then block to the line before NewTask if ioCreator has not been set... thus removing all unnecessary NewCreator calls..?

Copy link
Copy Markdown
Contributor Author

@gongguan gongguan Sep 7, 2020

Choose a reason for hiding this comment

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

suggest moving this if then block to the line before NewTask if ioCreator has not been set... thus removing all unnecessary NewCreator calls..?

@mikebrow adopt your suggestion, PTAL. I have to cleanup some code at the same time because windows didn't take care of StdinCloser and logURI.

Copy link
Copy Markdown
Contributor Author

@gongguan gongguan Sep 10, 2020

Choose a reason for hiding this comment

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

I finally choose removing unnecessary NewCreator in NewTask because it won't change too much.

@gongguan gongguan force-pushed the creator branch 3 times, most recently from 33a32b0 to b107c73 Compare September 7, 2020 06:52
@gongguan gongguan changed the title avoid building ioCreator twice when con not nil avoid unnecessary NewCreator calls Sep 7, 2020
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Sep 7, 2020

Build succeeded.

@gongguan gongguan force-pushed the creator branch 4 times, most recently from 084b23c to 15ad582 Compare September 10, 2020 07:29
@gongguan
Copy link
Copy Markdown
Contributor Author

gongguan commented Sep 10, 2020

PTAL @AkihiroSuda @dmcgowan

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Sep 10, 2020

Build succeeded.

mikebrow
mikebrow approved these changes Sep 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

pls address DCO..

@gongguan
Copy link
Copy Markdown
Contributor Author

gongguan commented Sep 21, 2020

pls address DCO..

Done, PTAL

estesp
estesp approved these changes Sep 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 534be84 into containerd:master Sep 21, 2020
@gongguan gongguan deleted the creator branch September 21, 2020 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

@mikebrow mikebrow mikebrow approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gongguan @estesp @mikebrow