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

fix(build): increase packing performance #28

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

Conversation

traa
Copy link

@traa traa commented Mar 26, 2019

Implementation is kinda hairy, but overall on large modules like mine (12+k files) it improves packing speed more than 10 times (from 350+ seconds to 20+ seconds on nwn-build pack-only)

Copy link
Owner

@jakkn jakkn left a comment

Choose a reason for hiding this comment

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

The changes work on my end and I see the performance improvement, but it broke pack for flat folder layout. See comments for explanation and suggested fix. Let me know if you need help with it.

gffs.each do |gff|
FileUtils.rm(gff) unless srcs.detect{|src| File.basename(gff) == File.basename(src)}
# extension is a name of the subfolder
type = File.extname(gff).delete('.')
Copy link
Owner

Choose a reason for hiding this comment

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

build.rb supports flat folder layout and the file matching here makes the below code blow up when running

nwn-build --flat extract
nwn-build pack-only

FLAT_LAYOUT = options[:flat] || (SOURCES.size > 0 && Pathname.glob(to_forward_slash SRC_DIR.join("*/")).size == 0) || false # Assume flat layout only on -f or if the source folder contains files but no directories

Array lookups, though slow, were safe in this regard. A quickfix could be an optional join on type, and to pass the value of FLAT_LAYOUT to pack.rake and apply the same logic there. A prettier fix might be to use a hash map, but that would require even more changes and I'm hesitant to rewrite large parts at this point due to the lack of test coverage.

puts "[INFO] packing changed file: %s" % File.basename(t.name)
# -i IN Input file [default: -]
# -o OUT Output file [default: -]
# -k OUTFORMAT Output format [default: autodetect]
Copy link
Owner

Choose a reason for hiding this comment

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

This reads nicely and is useful. Thumbs up.

puts "[INFO] No change in nss sources detected. Skipping compilation."
end

compile_nss(MODULE_FILE) unless skip_compile
Copy link
Owner

Choose a reason for hiding this comment

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

The logic above doesn't look right. This function was a mess already, and I think it needs to be redone in a more sensible way. The two flags make it complex, and there's something not right about conditionals that log an action without actually doing anything.

I would suggest replacing L461-468 with

  nss_changed = update_gffs()
  should_compile = nss_changed && !skip_compile

And this line to

Suggested change
compile_nss(MODULE_FILE) unless skip_compile
if should_compile
compile_nss(MODULE_FILE)
else
if skip_compile
puts "[INFO] Skipping compilation due to configuration"
else
puts "[INFO] No change in nss sources detected. Skipping compilation."
end
end

Or perhaps you see something more logical and/or compact.

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.

2 participants