Basic file locking.
If someone's already doing dex2oat, wait for them to finish rather
than duplicate their work.
Change-Id: Id620c75e013cbbcc84f5bf62a7629533a95307df
diff --git a/build/Android.oat.mk b/build/Android.oat.mk
index 50c05ab..6d72ded 100644
--- a/build/Android.oat.mk
+++ b/build/Android.oat.mk
@@ -49,11 +49,13 @@
$(HOST_CORE_OAT): $(HOST_CORE_DEX) $(DEX2OAT_DEPENDENCY)
@echo "host dex2oat: $@ ($?)"
@mkdir -p $(dir $@)
+ @rm -f $@
$(hide) $(DEX2OAT) --runtime-arg -Xms16m --runtime-arg -Xmx16m $(addprefix --dex-file=,$(filter-out $(DEX2OAT),$^)) --oat=$@ --image=$(HOST_CORE_IMG) --base=$(IMG_HOST_BASE_ADDRESS)
$(TARGET_CORE_OAT): $(TARGET_CORE_DEX) $(DEX2OAT_DEPENDENCY)
@echo "target dex2oat: $@ ($?)"
@mkdir -p $(dir $@)
+ @rm -f $@
$(hide) $(DEX2OAT) --runtime-arg -Xms32m --runtime-arg -Xmx32m $(addprefix --dex-file=,$(filter-out $(DEX2OAT),$^)) --oat=$@ --image=$(TARGET_CORE_IMG) --base=$(IMG_TARGET_BASE_ADDRESS) --host-prefix=$(PRODUCT_OUT)
$(HOST_CORE_IMG): $(HOST_CORE_OAT)
@@ -70,6 +72,7 @@
$(TARGET_BOOT_OAT): $(TARGET_BOOT_DEX) $(DEX2OAT_DEPENDENCY)
@echo "target dex2oat: $@ ($?)"
@mkdir -p $(dir $@)
+ @rm -f $@
$(hide) $(DEX2OAT) --runtime-arg -Xms256m --runtime-arg -Xmx256m $(addprefix --dex-file=,$(filter-out $(DEX2OAT),$^)) --oat=$@ --image=$(TARGET_BOOT_IMG) --base=$(IMG_TARGET_BASE_ADDRESS) --host-prefix=$(PRODUCT_OUT)
$(TARGET_BOOT_IMG): $(TARGET_BOOT_OAT)
diff --git a/build/Android.oattest.mk b/build/Android.oattest.mk
index bb6695e..7d467bc 100644
--- a/build/Android.oattest.mk
+++ b/build/Android.oattest.mk
@@ -40,6 +40,7 @@
define build-art-oat
$(2): $(1) $(3) $(DEX2OAT_DEPENDENCY)
@echo "target dex2oat: $$@ ($$?)"
+ @rm -f $$@
$(hide) $(DEX2OAT) --runtime-arg -Xms64m --runtime-arg -Xmx64m --boot-image=$(3) $(addprefix --dex-file=,$$<) --oat=$$@ --host-prefix=$(PRODUCT_OUT)
endef
diff --git a/src/class_linker.cc b/src/class_linker.cc
index a94607a..4f387db 100644
--- a/src/class_linker.cc
+++ b/src/class_linker.cc
@@ -568,8 +568,12 @@
std::string oat_file_option("--oat=");
oat_file_option += oat_filename;
- execl("/system/bin/dex2oatd",
- "/system/bin/dex2oatd",
+ std::string dex2oat("/system/bin/dex2oat");
+#ifndef NDEBUG
+ dex2oat += 'd';
+#endif
+
+ execl(dex2oat.c_str(), dex2oat.c_str(),
"--runtime-arg", "-Xms64m",
"--runtime-arg", "-Xmx64m",
boot_image_option.c_str(),
@@ -636,6 +640,9 @@
}
LOG(WARNING) << ".oat file " << oat_file->GetLocation()
<< " is older than " << dex_file.GetLocation() << " --- regenerating";
+ if (TEMP_FAILURE_RETRY(unlink(oat_file->GetLocation().c_str())) != 0) {
+ PLOG(FATAL) << "Couldn't remove obsolete .oat file " << oat_file->GetLocation();
+ }
// Fall through...
}
// Generate oat file if it wasn't found or was obsolete.
diff --git a/src/common_test.h b/src/common_test.h
index 928cb51..3966742 100644
--- a/src/common_test.h
+++ b/src/common_test.h
@@ -56,6 +56,7 @@
filename_ += "/TmpFile-XXXXXX";
fd_ = mkstemp(&filename_[0]);
CHECK_NE(-1, fd_);
+ file_.reset(OS::FileFromFd(GetFilename(), fd_));
}
~ScratchFile() {
@@ -69,6 +70,10 @@
return filename_.c_str();
}
+ File* GetFile() const {
+ return file_.get();
+ }
+
int GetFd() const {
return fd_;
}
@@ -76,6 +81,7 @@
private:
std::string filename_;
int fd_;
+ UniquePtr<File> file_;
};
class CommonTest : public testing::Test {
diff --git a/src/dex2oat.cc b/src/dex2oat.cc
index 80b21e2..f05f99a 100644
--- a/src/dex2oat.cc
+++ b/src/dex2oat.cc
@@ -2,6 +2,7 @@
#include <stdio.h>
#include <stdlib.h>
+#include <sys/file.h>
#include <string>
#include <vector>
@@ -62,6 +63,31 @@
exit(EXIT_FAILURE);
}
+class FileJanitor {
+public:
+ FileJanitor(const std::string& filename, int fd)
+ : filename_(filename), fd_(fd), do_unlink_(true) {
+ }
+
+ void KeepFile() {
+ do_unlink_ = false;
+ }
+
+ ~FileJanitor() {
+ if (fd_ != -1) {
+ flock(fd_, LOCK_UN);
+ }
+ if (do_unlink_) {
+ unlink(filename_.c_str());
+ }
+ }
+
+private:
+ std::string filename_;
+ int fd_;
+ bool do_unlink_;
+};
+
int dex2oat(int argc, char** argv) {
// Skip over argv[0].
argv++;
@@ -143,17 +169,62 @@
}
}
- // Check early that the result of compilation can be written
- // TODO: implement a proper locking scheme here, probably hold onto the open file..
- if (OS::FileExists(oat_filename.c_str())) {
- // File exists, check we can write to it
- UniquePtr<File> file(OS::OpenFile(oat_filename.c_str(), true));
- if (file.get() == NULL) {
+ // Create the output file if we can, or open it read-only if we weren't first.
+ bool did_create = true;
+ int fd = open(oat_filename.c_str(), O_EXCL | O_CREAT | O_TRUNC | O_RDWR, 0666);
+ if (fd == -1) {
+ if (errno != EEXIST) {
PLOG(ERROR) << "Unable to create oat file " << oat_filename;
return EXIT_FAILURE;
}
+ did_create = false;
+ fd = open(oat_filename.c_str(), O_RDONLY);
+ if (fd == -1) {
+ PLOG(ERROR) << "Unable to open oat file for reading " << oat_filename;
+ return EXIT_FAILURE;
+ }
}
- LOG(INFO) << "dex2oat: " << oat_filename;
+
+ // Handles removing the file on failure and unlocking on both failure and success.
+ FileJanitor file_janitor(oat_filename, fd);
+
+ // TODO: TEMP_FAILURE_RETRY on flock calls
+
+ // If we won the creation race, block trying to take the lock (since we're going to be doing
+ // the work, we need the lock). If we lost the creation race, spin trying to take the lock
+ // non-blocking until we fail -- at which point we know the other guy has the lock -- and then
+ // block trying to take the now-taken lock.
+ if (did_create) {
+ LOG(INFO) << "This process created " << oat_filename;
+ while (flock(fd, LOCK_EX) != 0) {
+ // Try again.
+ }
+ LOG(INFO) << "This process created and locked " << oat_filename;
+ } else {
+ LOG(INFO) << "Another process has already created " << oat_filename;
+ while (flock(fd, LOCK_EX | LOCK_NB) == 0) {
+ // Give up the lock and hope the creator has taken the lock next time round.
+ flock(fd, LOCK_UN);
+ }
+ // Now a non-blocking attempt to take the lock has failed, we know the other guy has the
+ // lock, so block waiting to take it.
+ LOG(INFO) << "Another process is already working on " << oat_filename;
+ if (flock(fd, LOCK_EX) != 0) {
+ PLOG(ERROR) << "Waiter unable to wait for creator to finish " << oat_filename;
+ return EXIT_FAILURE;
+ }
+ // We have the lock and the creator has finished.
+ // TODO: check the creator did a good job by checking the header.
+ LOG(INFO) << "Another process finished working on " << oat_filename;
+ // Job done.
+ file_janitor.KeepFile();
+ return EXIT_SUCCESS;
+ }
+
+ // If we get this far, we won the creation race and have locked the file.
+ UniquePtr<File> oat_file(OS::FileFromFd(oat_filename.c_str(), fd));
+
+ LOG(INFO) << "dex2oat: " << oat_file->name();
Runtime::Options options;
options.push_back(std::make_pair("compiler", reinterpret_cast<void*>(NULL)));
@@ -266,22 +337,26 @@
}
}
- if (!OatWriter::Create(oat_filename, class_loader.get(), compiler)) {
- LOG(ERROR) << "Failed to create oat file " << oat_filename;
+ if (!OatWriter::Create(oat_file.get(), class_loader.get(), compiler)) {
+ LOG(ERROR) << "Failed to create oat file " << oat_file->name();
return EXIT_FAILURE;
}
if (image_filename == NULL) {
+ LOG(INFO) << "No image filename supplied; exiting";
return EXIT_SUCCESS;
}
CHECK(compiler.IsImage());
ImageWriter image_writer;
- if (!image_writer.Write(image_filename, image_base, oat_filename, host_prefix)) {
+ if (!image_writer.Write(image_filename, image_base, oat_file->name(), host_prefix)) {
LOG(ERROR) << "Failed to create image file " << image_filename;
return EXIT_FAILURE;
}
+ // We wrote the file successfully, and want to keep it.
+ LOG(INFO) << "Image written successfully " << image_filename;
+ file_janitor.KeepFile();
return EXIT_SUCCESS;
}
diff --git a/src/image_test.cc b/src/image_test.cc
index 2ef7489..3e64d95 100644
--- a/src/image_test.cc
+++ b/src/image_test.cc
@@ -18,7 +18,7 @@
TEST_F(ImageTest, WriteRead) {
ScratchFile tmp_oat;
- bool success_oat = OatWriter::Create(tmp_oat.GetFilename(), NULL, *compiler_.get());
+ bool success_oat = OatWriter::Create(tmp_oat.GetFile(), NULL, *compiler_.get());
ASSERT_TRUE(success_oat);
ImageWriter writer;
diff --git a/src/oat_test.cc b/src/oat_test.cc
index 3433e44..e90d2ed 100644
--- a/src/oat_test.cc
+++ b/src/oat_test.cc
@@ -19,7 +19,7 @@
}
ScratchFile tmp;
- bool success = OatWriter::Create(tmp.GetFilename(), class_loader.get(), *compiler_.get());
+ bool success = OatWriter::Create(tmp.GetFile(), class_loader.get(), *compiler_.get());
ASSERT_TRUE(success);
if (compile) { // OatWriter strips the code, regenerate to compare
diff --git a/src/oat_writer.cc b/src/oat_writer.cc
index 214d177..664dcfe 100644
--- a/src/oat_writer.cc
+++ b/src/oat_writer.cc
@@ -10,12 +10,12 @@
namespace art {
-bool OatWriter::Create(const std::string& filename,
+bool OatWriter::Create(File* file,
const ClassLoader* class_loader,
const Compiler& compiler) {
const std::vector<const DexFile*>& dex_files = ClassLoader::GetCompileTimeClassPath(class_loader);
OatWriter oat_writer(dex_files, class_loader, compiler);
- return oat_writer.Write(filename);
+ return oat_writer.Write(file);
}
OatWriter::OatWriter(const std::vector<const DexFile*>& dex_files,
@@ -293,32 +293,26 @@
#define DCHECK_CODE_OFFSET() \
DCHECK_EQ(static_cast<off_t>(code_offset), lseek(file->Fd(), 0, SEEK_CUR))
-bool OatWriter::Write(const std::string& filename) {
- UniquePtr<File> file(OS::OpenFile(filename.c_str(), true));
- if (file.get() == NULL) {
- PLOG(ERROR) << "Failed to open file " << filename << " for writing";
- return false;
- }
-
+bool OatWriter::Write(File* file) {
if (!file->WriteFully(oat_header_, sizeof(*oat_header_))) {
- PLOG(ERROR) << "Failed to write oat header to " << filename;
+ PLOG(ERROR) << "Failed to write oat header to " << file->name();
return false;
}
- if (!WriteTables(file.get())) {
- LOG(ERROR) << "Failed to write oat tables to " << filename;
+ if (!WriteTables(file)) {
+ LOG(ERROR) << "Failed to write oat tables to " << file->name();
return false;
}
- size_t code_offset = WriteCode(file.get());
+ size_t code_offset = WriteCode(file);
if (code_offset == 0) {
- LOG(ERROR) << "Failed to write oat code to " << filename;
+ LOG(ERROR) << "Failed to write oat code to " << file->name();
return false;
}
- code_offset = WriteCodeDexFiles(file.get(), code_offset);
+ code_offset = WriteCodeDexFiles(file, code_offset);
if (code_offset == 0) {
- LOG(ERROR) << "Failed to write oat code for dex files to " << filename;
+ LOG(ERROR) << "Failed to write oat code for dex files to " << file->name();
return false;
}
@@ -328,19 +322,19 @@
bool OatWriter::WriteTables(File* file) {
for (size_t i = 0; i != oat_dex_files_.size(); ++i) {
if (!oat_dex_files_[i]->Write(file)) {
- PLOG(ERROR) << "Failed to write oat dex information";
+ PLOG(ERROR) << "Failed to write oat dex information to " << file->name();
return false;
}
}
for (size_t i = 0; i != oat_classes_.size(); ++i) {
if (!oat_classes_[i]->Write(file)) {
- PLOG(ERROR) << "Failed to write oat classes information";
+ PLOG(ERROR) << "Failed to write oat classes information to " << file->name();
return false;
}
}
for (size_t i = 0; i != oat_methods_.size(); ++i) {
if (!oat_methods_[i]->Write(file)) {
- PLOG(ERROR) << "Failed to write oat methods information";
+ PLOG(ERROR) << "Failed to write oat methods information to " << file->name();
return false;
}
}
@@ -352,7 +346,7 @@
off_t new_offset = lseek(file->Fd(), executable_offset_padding_length_, SEEK_CUR);
if (static_cast<uint32_t>(new_offset) != code_offset) {
PLOG(ERROR) << "Failed to seek to oat code section. Actual: " << new_offset
- << " Expected: " << code_offset;
+ << " Expected: " << code_offset << " File: " << file->name();
return 0;
}
DCHECK_CODE_OFFSET();
@@ -371,12 +365,8 @@
return code_offset;
}
-size_t OatWriter::WriteCodeDexFile(File* file,
- size_t code_offset,
- const DexFile& dex_file) {
- for (size_t class_def_index = 0;
- class_def_index < dex_file.NumClassDefs();
- class_def_index++) {
+size_t OatWriter::WriteCodeDexFile(File* file, size_t code_offset, const DexFile& dex_file) {
+ for (size_t class_def_index = 0; class_def_index < dex_file.NumClassDefs(); class_def_index++) {
const DexFile::ClassDef& class_def = dex_file.GetClassDef(class_def_index);
code_offset = WriteCodeClassDef(file, code_offset, dex_file, class_def);
if (code_offset == 0) {
@@ -386,6 +376,10 @@
return code_offset;
}
+void OatWriter::ReportWriteFailure(const char* what, Method* m, File* f) {
+ PLOG(ERROR) << "Failed to write " << what << " for " << PrettyMethod(m) << " to " << f->name();
+}
+
size_t OatWriter::WriteCodeClassDef(File* file,
size_t code_offset,
const DexFile& dex_file,
@@ -436,7 +430,7 @@
off_t new_offset = lseek(file->Fd(), aligned_code_delta, SEEK_CUR);
if (static_cast<uint32_t>(new_offset) != aligned_code_offset) {
PLOG(ERROR) << "Failed to seek to align oat code. Actual: " << new_offset
- << " Expected: " << aligned_code_offset;
+ << " Expected: " << aligned_code_offset << " File: " << file->name();
return false;
}
code_offset += aligned_code_delta;
@@ -456,7 +450,7 @@
DCHECK((code_size == 0 && method->GetOatCodeOffset() == 0)
|| offset == method->GetOatCodeOffset()) << PrettyMethod(method);
if (!file->WriteFully(&code[0], code_size)) {
- PLOG(ERROR) << "Failed to write method code for " << PrettyMethod(method);
+ ReportWriteFailure("method code", method, file);
return false;
}
code_offset += code_size;
@@ -469,22 +463,22 @@
uint32_t core_spill_mask = method->GetCoreSpillMask();
uint32_t fp_spill_mask = method->GetFpSpillMask();
if (!file->WriteFully(&frame_size_in_bytes, sizeof(frame_size_in_bytes))) {
- PLOG(ERROR) << "Failed to write method frame size for " << PrettyMethod(method);
+ ReportWriteFailure("method frame size", method, file);
return false;
}
code_offset += sizeof(frame_size_in_bytes);
if (!file->WriteFully(&return_pc_offset_in_bytes, sizeof(return_pc_offset_in_bytes))) {
- PLOG(ERROR) << "Failed to write method return pc offset for " << PrettyMethod(method);
+ ReportWriteFailure("method return pc", method, file);
return false;
}
code_offset += sizeof(return_pc_offset_in_bytes);
if (!file->WriteFully(&core_spill_mask, sizeof(core_spill_mask))) {
- PLOG(ERROR) << "Failed to write method core spill mask for " << PrettyMethod(method);
+ ReportWriteFailure("method core spill mask", method, file);
return false;
}
code_offset += sizeof(core_spill_mask);
if (!file->WriteFully(&fp_spill_mask, sizeof(fp_spill_mask))) {
- PLOG(ERROR) << "Failed to write method fp spill mask for " << PrettyMethod(method);
+ ReportWriteFailure("method fp spill mask", method, file);
return false;
}
code_offset += sizeof(fp_spill_mask);
@@ -502,7 +496,7 @@
DCHECK((mapping_table_size == 0 && method->GetOatMappingTableOffset() == 0)
|| code_offset == method->GetOatMappingTableOffset()) << PrettyMethod(method);
if (!file->WriteFully(&mapping_table[0], mapping_table_size)) {
- PLOG(ERROR) << "Failed to write mapping table for " << PrettyMethod(method);
+ ReportWriteFailure("mapping table", method, file);
return false;
}
code_offset += mapping_table_size;
@@ -521,7 +515,7 @@
DCHECK((vmap_table_size == 0 && method->GetOatVmapTableOffset() == 0)
|| code_offset == method->GetOatVmapTableOffset()) << PrettyMethod(method);
if (!file->WriteFully(&vmap_table[0], vmap_table_size)) {
- PLOG(ERROR) << "Failed to write vmap table for " << PrettyMethod(method);
+ ReportWriteFailure("vmap table", method, file);
return false;
}
code_offset += vmap_table_size;
@@ -557,7 +551,7 @@
DCHECK((invoke_stub_size == 0 && method->GetOatInvokeStubOffset() == 0)
|| code_offset == method->GetOatInvokeStubOffset()) << PrettyMethod(method);
if (!file->WriteFully(&invoke_stub[0], invoke_stub_size)) {
- PLOG(ERROR) << "Failed to write invoke stub code for " << PrettyMethod(method);
+ ReportWriteFailure("invoke stub code", method, file);
return false;
}
code_offset += invoke_stub_size;
@@ -598,19 +592,19 @@
bool OatWriter::OatDexFile::Write(File* file) const {
if (!file->WriteFully(&dex_file_location_size_, sizeof(dex_file_location_size_))) {
- PLOG(ERROR) << "Failed to write dex file location length";
+ PLOG(ERROR) << "Failed to write dex file location length to " << file->name();
return false;
}
if (!file->WriteFully(dex_file_location_data_, dex_file_location_size_)) {
- PLOG(ERROR) << "Failed to write dex file location data";
+ PLOG(ERROR) << "Failed to write dex file location data to " << file->name();
return false;
}
if (!file->WriteFully(&dex_file_checksum_, sizeof(dex_file_checksum_))) {
- PLOG(ERROR) << "Failed to write dex file checksum";
+ PLOG(ERROR) << "Failed to write dex file checksum to " << file->name();
return false;
}
if (!file->WriteFully(&classes_offset_, sizeof(classes_offset_))) {
- PLOG(ERROR) << "Failed to write classes offset";
+ PLOG(ERROR) << "Failed to write classes offset to " << file->name();
return false;
}
return true;
@@ -630,7 +624,7 @@
bool OatWriter::OatClasses::Write(File* file) const {
if (!file->WriteFully(&methods_offsets_[0], SizeOf())) {
- PLOG(ERROR) << "Failed to methods offsets";
+ PLOG(ERROR) << "Failed to write methods offsets to " << file->name();
return false;
}
return true;
@@ -650,7 +644,7 @@
bool OatWriter::OatMethods::Write(File* file) const {
if (!file->WriteFully(&method_offsets_[0], SizeOf())) {
- PLOG(ERROR) << "Failed to method offsets";
+ PLOG(ERROR) << "Failed to write method offsets to " << file->name();
return false;
}
return true;
diff --git a/src/oat_writer.h b/src/oat_writer.h
index 1c7ee9a..0642fa4 100644
--- a/src/oat_writer.h
+++ b/src/oat_writer.h
@@ -49,9 +49,7 @@
class OatWriter {
public:
// Write an oat file. Returns true on success, false on failure.
- static bool Create(const std::string& filename,
- const ClassLoader* class_loader,
- const Compiler& compiler);
+ static bool Create(File* file, const ClassLoader* class_loader, const Compiler& compiler);
private:
@@ -78,7 +76,7 @@
size_t class_def_method_index,
Method* method);
- bool Write(const std::string& filename);
+ bool Write(File* file);
bool WriteTables(File* file);
size_t WriteCode(File* file);
size_t WriteCodeDexFiles(File* file,
@@ -94,6 +92,8 @@
size_t offset,
Method* method);
+ void ReportWriteFailure(const char* what, Method* m, File* f);
+
class OatDexFile {
public:
explicit OatDexFile(const DexFile& dex_file);
diff --git a/src/oatopt.cc b/src/oatopt.cc
index c792b4d..dd702d4 100644
--- a/src/oatopt.cc
+++ b/src/oatopt.cc
@@ -49,8 +49,12 @@
std::string oat_file_option("--oat=");
oat_file_option += GetArtCacheFilenameOrDie(OatFile::DexFilenameToOatFilename(dex_file.get()->GetLocation()));
- execl("/system/bin/dex2oatd",
- "/system/bin/dex2oatd",
+ std::string dex2oat("/system/bin/dex2oat");
+#ifndef NDEBUG
+ dex2oat += 'd';
+#endif
+
+ execl(dex2oat.c_str(), dex2oat.c_str(),
"--runtime-arg", "-Xms64m",
"--runtime-arg", "-Xmx64m",
"--boot-image=/data/art-cache/boot.art",