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

Adding malloc_trim call on full GC #5069

Closed
wants to merge 5 commits into from
Closed

Conversation

RoryO
Copy link
Contributor

@RoryO RoryO commented Aug 30, 2020

Follow up on #5047. Does a couple things:

  • Adds a few methods to MRuby::Command::Compiler for asking about headers and functions. I took inspiration from how meson does it, which has a nice API.
  • Wraps the necessary includes and call within a #define set inside gcc.rake. That way if a user has a custom build_config.rb they can remove the define if they wish, like if they are using a different allocator than glib.

@@ -162,6 +188,46 @@ def get_dependencies(file)
end
deps << MRUBY_CONFIG
end

def test_code_template(function: nil, header: nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is kind of ugly. I didn't know how far back we support Ruby so I didn't use the tilde <<~ form of heredocs to make it a bit less ugly.

@@ -126,6 +127,31 @@ def define_rules(build_dir, source_dir='')
end
end

def compiles?(source_text)
Copy link
Contributor Author

@RoryO RoryO Aug 30, 2020

Choose a reason for hiding this comment

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

@matz I hope this is sufficient for what you were thinking about adding autoconf like behavior when we need it.

# Change to a tmp dir when compiling so we don't litter compiler artifacts
Dir.mktmpdir do |tmpdir|
Dir.chdir tmpdir
sh(command, infile.path, verbose: false) { |retval, _| is_success = retval }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a bit of a nuisance here. You cannot silence stderr with the Rake sh method. When testing for features it may be a legitimate failure, which is why we're asking the compiler. Unfortunately gcc and clang output the compiler errors to stderr. Doing a shell redirection isn't cross platform. We could use Open3.capture3 instead of sh, I didn't know if it is okay to require and use.

@RoryO
Copy link
Contributor Author

RoryO commented Aug 30, 2020

It appears either mingw or the CI system doesn't allow removing temp directories. That is a strange situation either way. I will work around it.

@matz
Copy link
Member

matz commented Aug 31, 2020

I am wondering whether it may be better to specify manually via a macro define, e.g. MRB_USE_MALLOC_TRIM.
In that case, we don't have to add a complex detection code. No worry about cross-compiling nor compiler portability.

@RoryO
Copy link
Contributor Author

RoryO commented Sep 1, 2020

Oh yes it should be prefixed with MRB_.

Are you saying you would like it off by default and enabling it with a custom build_config?

@matz
Copy link
Member

matz commented Sep 1, 2020

@RoryO yes. Altough it's less intelligent. I'd like to follow KISS principle here.

@RoryO
Copy link
Contributor Author

RoryO commented Sep 1, 2020

Okay, I have done that.

@matz
Copy link
Member

matz commented Sep 2, 2020

Thank you! It will be merged to mruby3 branch.

@RoryO RoryO changed the base branch from master to mruby3 September 2, 2020 01:21
@RoryO
Copy link
Contributor Author

RoryO commented Sep 2, 2020

Oh that’s new! Okay I changed the base branch.

@matz
Copy link
Member

matz commented Sep 2, 2020

Oh, you didn't have to. I did it on my side.

@matz
Copy link
Member

matz commented Sep 2, 2020

Thank you anyway.

@RoryO
Copy link
Contributor Author

RoryO commented Sep 2, 2020

Should this be closed then?

@matz
Copy link
Member

matz commented Sep 2, 2020

I will close when I push to GitHub. I am struggling with other issues. Wait for a while.

matz added a commit that referenced this pull request Sep 3, 2020
@matz
Copy link
Member

matz commented Sep 4, 2020

Merged.

@matz matz closed this Sep 4, 2020
matz added a commit that referenced this pull request Sep 17, 2020
matz added a commit that referenced this pull request Sep 18, 2020
matz added a commit that referenced this pull request Sep 29, 2020
matz added a commit that referenced this pull request Oct 14, 2020
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.

None yet

2 participants