ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer - #14261

Closed
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail
Closed

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer#14261
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail

Conversation

@js8544

@js8544js8544 commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

Java's buffer type doesn't support reserve, which causes java-jars to fail.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has no components in JIRA, make sure you assign one.

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@github-actions

Copy link
Copy Markdown

Revision: 7f6d36cdbbf89f56f53e132198c86b0c004d119d

Submitted crossbow builds: ursacomputing/crossbow @ actions-d26b84437c

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

1 similar comment
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@kou Hi kou, sorry to bother. I requested two crossbow builds today but they were ignored by the bot. Do you have any idea why this happened?

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 263c2956ee7c09ab1e74fc5e5329e0c55e28d84c

Submitted crossbow builds: ursacomputing/crossbow @ actions-7ce585f8a8

TaskStatus
java-jarsGithub Actions

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

Hmm... It's strange...

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 1e7610799c2ee7724bb7a225bdfd2cafd0e811c5

Submitted crossbow builds: ursacomputing/crossbow @ actions-478721e321

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

Brew's LLVM 15 enables z3 by default, and it's shared linked. So at task Build Java Jars, it's required to have libz3 installed on the system. There are several ways to solve this:

  1. Add a brew install z3 task to Build Java Jars task
  2. Pin LLVM for java to 14. Currently it uses arrow/cpp/Brewfile. We can create another Brewfile in arrow/java and pin it in the new file.
  3. We can also pin LLVM to 14 for all MacOS builds at arrow/cpp/Brewfile. Because LLVM 15 is noticably slower than LLVM 14 somehow. It can also speed up the build time for other MacOS CI tasks.

Which way is preferred? cc @kou@pitrou

@kou

kou commented Sep 30, 2022

Copy link
Copy Markdown
Member
  1. for now but we need a new Jira issue for unpinning.

@js8544

Copy link
Copy Markdown
ContributorAuthor
  1. for now but we need a new Jira issue for unpinning.

I've pinned Brew llvm version to 14. Also filed https://issues.apache.org/jira/browse/ARROW-17902 to track current workarounds for LLVM 15 related issues.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 39437b059b466a00de6778034610600de6e15d0c

Submitted crossbow builds: ursacomputing/crossbow @ actions-e128cce1c4

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: dfb7e2d71450a3bff7191d2dda633c85d618b035

Submitted crossbow builds: ursacomputing/crossbow @ actions-77a61b58b9

TaskStatus
java-jarsGithub Actions

@js8544
js8544force-pushed the jinshang/allow_reserve_to_fail branch from dfb7e2d to 5710f8bCompareSeptember 30, 2022 08:10
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 5710f8b

Submitted crossbow builds: ursacomputing/crossbow @ actions-632d4e01ac

TaskStatus
java-jarsGithub Actions

@pitrou

Copy link
Copy Markdown
Member

Hmm... it would be much more desirable to implement Reserve on Java-exported buffers instead. @lwhite1@lidavidm Could one of you help perhaps?

@lidavidm

Copy link
Copy Markdown
Member

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

@js8544

Copy link
Copy Markdown
ContributorAuthor

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

Because the resize implementation for SimpleBuffer doesn't amotize the realloc costs, if we don't reserve, there will be N reallocs, one for each resize call. Now it's reduced to log2(N), since with each reserve we double the buffer size. We still need to call resize to track the actual size.

@lidavidm

Copy link
Copy Markdown
Member

Ah, thanks for the explanation. I think it would be best if we could implement reserve for Java-allocated buffers, but I am not familiar with the code and don't have time to look at it.

@lidavidm

Copy link
Copy Markdown
Member

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

@js8544

Copy link
Copy Markdown
ContributorAuthor

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

It seems like we can. I can take a look later today if you don't have time for it.

@js8544js8544 changed the title ARROW-17869: [Java][Gandiva] Allow reserve not implemented in varlen output bufferARROW-17869: [Java][Gandiva] Implement Reserve for JavaBufferOct 4, 2022
@js8544js8544 closed this Oct 5, 2022
@pitrou

Copy link
Copy Markdown
Member

@js8544 Are you closing this PR in favor of another one?

@js8544

Copy link
Copy Markdown
ContributorAuthor

@js8544 Are you closing this PR in favor of another one?

@pitrou Yes, I will submit another PR once #14310 is merged since this change requires llvm@14.

pitrou pushed a commit that referenced this pull request Oct 11, 2022
See previous discussions at #14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@js8544
js8544 deleted the jinshang/allow_reserve_to_fail branch October 11, 2022 08:43
kou pushed a commit to apache/arrow-java that referenced this pull request Nov 25, 2024
See previous discussions at apache/arrow#14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@js8544@kou@pitrou@lidavidm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer - #14261

Closed
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail
Closed

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer#14261
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail

Conversation

@js8544

@js8544js8544 commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

Java's buffer type doesn't support reserve, which causes java-jars to fail.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has no components in JIRA, make sure you assign one.

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@github-actions

Copy link
Copy Markdown

Revision: 7f6d36cdbbf89f56f53e132198c86b0c004d119d

Submitted crossbow builds: ursacomputing/crossbow @ actions-d26b84437c

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

1 similar comment
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@kou Hi kou, sorry to bother. I requested two crossbow builds today but they were ignored by the bot. Do you have any idea why this happened?

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 263c2956ee7c09ab1e74fc5e5329e0c55e28d84c

Submitted crossbow builds: ursacomputing/crossbow @ actions-7ce585f8a8

TaskStatus
java-jarsGithub Actions

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

Hmm... It's strange...

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 1e7610799c2ee7724bb7a225bdfd2cafd0e811c5

Submitted crossbow builds: ursacomputing/crossbow @ actions-478721e321

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

Brew's LLVM 15 enables z3 by default, and it's shared linked. So at task Build Java Jars, it's required to have libz3 installed on the system. There are several ways to solve this:

  1. Add a brew install z3 task to Build Java Jars task
  2. Pin LLVM for java to 14. Currently it uses arrow/cpp/Brewfile. We can create another Brewfile in arrow/java and pin it in the new file.
  3. We can also pin LLVM to 14 for all MacOS builds at arrow/cpp/Brewfile. Because LLVM 15 is noticably slower than LLVM 14 somehow. It can also speed up the build time for other MacOS CI tasks.

Which way is preferred? cc @kou@pitrou

@kou

kou commented Sep 30, 2022

Copy link
Copy Markdown
Member
  1. for now but we need a new Jira issue for unpinning.

@js8544

Copy link
Copy Markdown
ContributorAuthor
  1. for now but we need a new Jira issue for unpinning.

I've pinned Brew llvm version to 14. Also filed https://issues.apache.org/jira/browse/ARROW-17902 to track current workarounds for LLVM 15 related issues.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 39437b059b466a00de6778034610600de6e15d0c

Submitted crossbow builds: ursacomputing/crossbow @ actions-e128cce1c4

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: dfb7e2d71450a3bff7191d2dda633c85d618b035

Submitted crossbow builds: ursacomputing/crossbow @ actions-77a61b58b9

TaskStatus
java-jarsGithub Actions

@js8544
js8544force-pushed the jinshang/allow_reserve_to_fail branch from dfb7e2d to 5710f8bCompareSeptember 30, 2022 08:10
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 5710f8b

Submitted crossbow builds: ursacomputing/crossbow @ actions-632d4e01ac

TaskStatus
java-jarsGithub Actions

@pitrou

Copy link
Copy Markdown
Member

Hmm... it would be much more desirable to implement Reserve on Java-exported buffers instead. @lwhite1@lidavidm Could one of you help perhaps?

@lidavidm

Copy link
Copy Markdown
Member

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

@js8544

Copy link
Copy Markdown
ContributorAuthor

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

Because the resize implementation for SimpleBuffer doesn't amotize the realloc costs, if we don't reserve, there will be N reallocs, one for each resize call. Now it's reduced to log2(N), since with each reserve we double the buffer size. We still need to call resize to track the actual size.

@lidavidm

Copy link
Copy Markdown
Member

Ah, thanks for the explanation. I think it would be best if we could implement reserve for Java-allocated buffers, but I am not familiar with the code and don't have time to look at it.

@lidavidm

Copy link
Copy Markdown
Member

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

@js8544

Copy link
Copy Markdown
ContributorAuthor

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

It seems like we can. I can take a look later today if you don't have time for it.

@js8544js8544 changed the title ARROW-17869: [Java][Gandiva] Allow reserve not implemented in varlen output bufferARROW-17869: [Java][Gandiva] Implement Reserve for JavaBufferOct 4, 2022
@js8544js8544 closed this Oct 5, 2022
@pitrou

Copy link
Copy Markdown
Member

@js8544 Are you closing this PR in favor of another one?

@js8544

Copy link
Copy Markdown
ContributorAuthor

@js8544 Are you closing this PR in favor of another one?

@pitrou Yes, I will submit another PR once #14310 is merged since this change requires llvm@14.

pitrou pushed a commit that referenced this pull request Oct 11, 2022
See previous discussions at #14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@js8544
js8544 deleted the jinshang/allow_reserve_to_fail branch October 11, 2022 08:43
kou pushed a commit to apache/arrow-java that referenced this pull request Nov 25, 2024
See previous discussions at apache/arrow#14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@js8544@kou@pitrou@lidavidm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer - #14261

Closed
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail
Closed

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer#14261
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail

Conversation

@js8544

@js8544js8544 commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

Java's buffer type doesn't support reserve, which causes java-jars to fail.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has no components in JIRA, make sure you assign one.

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@github-actions

Copy link
Copy Markdown

Revision: 7f6d36cdbbf89f56f53e132198c86b0c004d119d

Submitted crossbow builds: ursacomputing/crossbow @ actions-d26b84437c

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

1 similar comment
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@kou Hi kou, sorry to bother. I requested two crossbow builds today but they were ignored by the bot. Do you have any idea why this happened?

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 263c2956ee7c09ab1e74fc5e5329e0c55e28d84c

Submitted crossbow builds: ursacomputing/crossbow @ actions-7ce585f8a8

TaskStatus
java-jarsGithub Actions

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

Hmm... It's strange...

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 1e7610799c2ee7724bb7a225bdfd2cafd0e811c5

Submitted crossbow builds: ursacomputing/crossbow @ actions-478721e321

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

Brew's LLVM 15 enables z3 by default, and it's shared linked. So at task Build Java Jars, it's required to have libz3 installed on the system. There are several ways to solve this:

  1. Add a brew install z3 task to Build Java Jars task
  2. Pin LLVM for java to 14. Currently it uses arrow/cpp/Brewfile. We can create another Brewfile in arrow/java and pin it in the new file.
  3. We can also pin LLVM to 14 for all MacOS builds at arrow/cpp/Brewfile. Because LLVM 15 is noticably slower than LLVM 14 somehow. It can also speed up the build time for other MacOS CI tasks.

Which way is preferred? cc @kou@pitrou

@kou

kou commented Sep 30, 2022

Copy link
Copy Markdown
Member
  1. for now but we need a new Jira issue for unpinning.

@js8544

Copy link
Copy Markdown
ContributorAuthor
  1. for now but we need a new Jira issue for unpinning.

I've pinned Brew llvm version to 14. Also filed https://issues.apache.org/jira/browse/ARROW-17902 to track current workarounds for LLVM 15 related issues.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 39437b059b466a00de6778034610600de6e15d0c

Submitted crossbow builds: ursacomputing/crossbow @ actions-e128cce1c4

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: dfb7e2d71450a3bff7191d2dda633c85d618b035

Submitted crossbow builds: ursacomputing/crossbow @ actions-77a61b58b9

TaskStatus
java-jarsGithub Actions

@js8544
js8544force-pushed the jinshang/allow_reserve_to_fail branch from dfb7e2d to 5710f8bCompareSeptember 30, 2022 08:10
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 5710f8b

Submitted crossbow builds: ursacomputing/crossbow @ actions-632d4e01ac

TaskStatus
java-jarsGithub Actions

@pitrou

Copy link
Copy Markdown
Member

Hmm... it would be much more desirable to implement Reserve on Java-exported buffers instead. @lwhite1@lidavidm Could one of you help perhaps?

@lidavidm

Copy link
Copy Markdown
Member

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

@js8544

Copy link
Copy Markdown
ContributorAuthor

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

Because the resize implementation for SimpleBuffer doesn't amotize the realloc costs, if we don't reserve, there will be N reallocs, one for each resize call. Now it's reduced to log2(N), since with each reserve we double the buffer size. We still need to call resize to track the actual size.

@lidavidm

Copy link
Copy Markdown
Member

Ah, thanks for the explanation. I think it would be best if we could implement reserve for Java-allocated buffers, but I am not familiar with the code and don't have time to look at it.

@lidavidm

Copy link
Copy Markdown
Member

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

@js8544

Copy link
Copy Markdown
ContributorAuthor

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

It seems like we can. I can take a look later today if you don't have time for it.

@js8544js8544 changed the title ARROW-17869: [Java][Gandiva] Allow reserve not implemented in varlen output bufferARROW-17869: [Java][Gandiva] Implement Reserve for JavaBufferOct 4, 2022
@js8544js8544 closed this Oct 5, 2022
@pitrou

Copy link
Copy Markdown
Member

@js8544 Are you closing this PR in favor of another one?

@js8544

Copy link
Copy Markdown
ContributorAuthor

@js8544 Are you closing this PR in favor of another one?

@pitrou Yes, I will submit another PR once #14310 is merged since this change requires llvm@14.

pitrou pushed a commit that referenced this pull request Oct 11, 2022
See previous discussions at #14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@js8544
js8544 deleted the jinshang/allow_reserve_to_fail branch October 11, 2022 08:43
kou pushed a commit to apache/arrow-java that referenced this pull request Nov 25, 2024
See previous discussions at apache/arrow#14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@js8544@kou@pitrou@lidavidm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer - #14261

Closed
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail
Closed

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer#14261
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail

Conversation

@js8544

@js8544js8544 commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

Java's buffer type doesn't support reserve, which causes java-jars to fail.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has no components in JIRA, make sure you assign one.

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@github-actions

Copy link
Copy Markdown

Revision: 7f6d36cdbbf89f56f53e132198c86b0c004d119d

Submitted crossbow builds: ursacomputing/crossbow @ actions-d26b84437c

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

1 similar comment
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@kou Hi kou, sorry to bother. I requested two crossbow builds today but they were ignored by the bot. Do you have any idea why this happened?

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 263c2956ee7c09ab1e74fc5e5329e0c55e28d84c

Submitted crossbow builds: ursacomputing/crossbow @ actions-7ce585f8a8

TaskStatus
java-jarsGithub Actions

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

Hmm... It's strange...

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 1e7610799c2ee7724bb7a225bdfd2cafd0e811c5

Submitted crossbow builds: ursacomputing/crossbow @ actions-478721e321

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

Brew's LLVM 15 enables z3 by default, and it's shared linked. So at task Build Java Jars, it's required to have libz3 installed on the system. There are several ways to solve this:

  1. Add a brew install z3 task to Build Java Jars task
  2. Pin LLVM for java to 14. Currently it uses arrow/cpp/Brewfile. We can create another Brewfile in arrow/java and pin it in the new file.
  3. We can also pin LLVM to 14 for all MacOS builds at arrow/cpp/Brewfile. Because LLVM 15 is noticably slower than LLVM 14 somehow. It can also speed up the build time for other MacOS CI tasks.

Which way is preferred? cc @kou@pitrou

@kou

kou commented Sep 30, 2022

Copy link
Copy Markdown
Member
  1. for now but we need a new Jira issue for unpinning.

@js8544

Copy link
Copy Markdown
ContributorAuthor
  1. for now but we need a new Jira issue for unpinning.

I've pinned Brew llvm version to 14. Also filed https://issues.apache.org/jira/browse/ARROW-17902 to track current workarounds for LLVM 15 related issues.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 39437b059b466a00de6778034610600de6e15d0c

Submitted crossbow builds: ursacomputing/crossbow @ actions-e128cce1c4

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: dfb7e2d71450a3bff7191d2dda633c85d618b035

Submitted crossbow builds: ursacomputing/crossbow @ actions-77a61b58b9

TaskStatus
java-jarsGithub Actions

@js8544
js8544force-pushed the jinshang/allow_reserve_to_fail branch from dfb7e2d to 5710f8bCompareSeptember 30, 2022 08:10
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 5710f8b

Submitted crossbow builds: ursacomputing/crossbow @ actions-632d4e01ac

TaskStatus
java-jarsGithub Actions

@pitrou

Copy link
Copy Markdown
Member

Hmm... it would be much more desirable to implement Reserve on Java-exported buffers instead. @lwhite1@lidavidm Could one of you help perhaps?

@lidavidm

Copy link
Copy Markdown
Member

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

@js8544

Copy link
Copy Markdown
ContributorAuthor

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

Because the resize implementation for SimpleBuffer doesn't amotize the realloc costs, if we don't reserve, there will be N reallocs, one for each resize call. Now it's reduced to log2(N), since with each reserve we double the buffer size. We still need to call resize to track the actual size.

@lidavidm

Copy link
Copy Markdown
Member

Ah, thanks for the explanation. I think it would be best if we could implement reserve for Java-allocated buffers, but I am not familiar with the code and don't have time to look at it.

@lidavidm

Copy link
Copy Markdown
Member

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

@js8544

Copy link
Copy Markdown
ContributorAuthor

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

It seems like we can. I can take a look later today if you don't have time for it.

@js8544js8544 changed the title ARROW-17869: [Java][Gandiva] Allow reserve not implemented in varlen output bufferARROW-17869: [Java][Gandiva] Implement Reserve for JavaBufferOct 4, 2022
@js8544js8544 closed this Oct 5, 2022
@pitrou

Copy link
Copy Markdown
Member

@js8544 Are you closing this PR in favor of another one?

@js8544

Copy link
Copy Markdown
ContributorAuthor

@js8544 Are you closing this PR in favor of another one?

@pitrou Yes, I will submit another PR once #14310 is merged since this change requires llvm@14.

pitrou pushed a commit that referenced this pull request Oct 11, 2022
See previous discussions at #14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@js8544
js8544 deleted the jinshang/allow_reserve_to_fail branch October 11, 2022 08:43
kou pushed a commit to apache/arrow-java that referenced this pull request Nov 25, 2024
See previous discussions at apache/arrow#14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@js8544@kou@pitrou@lidavidm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer - #14261

Closed
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail
Closed

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer#14261
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail

Conversation

@js8544

@js8544js8544 commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

Java's buffer type doesn't support reserve, which causes java-jars to fail.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has no components in JIRA, make sure you assign one.

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@github-actions

Copy link
Copy Markdown

Revision: 7f6d36cdbbf89f56f53e132198c86b0c004d119d

Submitted crossbow builds: ursacomputing/crossbow @ actions-d26b84437c

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

1 similar comment
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@kou Hi kou, sorry to bother. I requested two crossbow builds today but they were ignored by the bot. Do you have any idea why this happened?

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 263c2956ee7c09ab1e74fc5e5329e0c55e28d84c

Submitted crossbow builds: ursacomputing/crossbow @ actions-7ce585f8a8

TaskStatus
java-jarsGithub Actions

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

Hmm... It's strange...

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 1e7610799c2ee7724bb7a225bdfd2cafd0e811c5

Submitted crossbow builds: ursacomputing/crossbow @ actions-478721e321

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

Brew's LLVM 15 enables z3 by default, and it's shared linked. So at task Build Java Jars, it's required to have libz3 installed on the system. There are several ways to solve this:

  1. Add a brew install z3 task to Build Java Jars task
  2. Pin LLVM for java to 14. Currently it uses arrow/cpp/Brewfile. We can create another Brewfile in arrow/java and pin it in the new file.
  3. We can also pin LLVM to 14 for all MacOS builds at arrow/cpp/Brewfile. Because LLVM 15 is noticably slower than LLVM 14 somehow. It can also speed up the build time for other MacOS CI tasks.

Which way is preferred? cc @kou@pitrou

@kou

kou commented Sep 30, 2022

Copy link
Copy Markdown
Member
  1. for now but we need a new Jira issue for unpinning.

@js8544

Copy link
Copy Markdown
ContributorAuthor
  1. for now but we need a new Jira issue for unpinning.

I've pinned Brew llvm version to 14. Also filed https://issues.apache.org/jira/browse/ARROW-17902 to track current workarounds for LLVM 15 related issues.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 39437b059b466a00de6778034610600de6e15d0c

Submitted crossbow builds: ursacomputing/crossbow @ actions-e128cce1c4

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: dfb7e2d71450a3bff7191d2dda633c85d618b035

Submitted crossbow builds: ursacomputing/crossbow @ actions-77a61b58b9

TaskStatus
java-jarsGithub Actions

@js8544
js8544force-pushed the jinshang/allow_reserve_to_fail branch from dfb7e2d to 5710f8bCompareSeptember 30, 2022 08:10
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 5710f8b

Submitted crossbow builds: ursacomputing/crossbow @ actions-632d4e01ac

TaskStatus
java-jarsGithub Actions

@pitrou

Copy link
Copy Markdown
Member

Hmm... it would be much more desirable to implement Reserve on Java-exported buffers instead. @lwhite1@lidavidm Could one of you help perhaps?

@lidavidm

Copy link
Copy Markdown
Member

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

@js8544

Copy link
Copy Markdown
ContributorAuthor

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

Because the resize implementation for SimpleBuffer doesn't amotize the realloc costs, if we don't reserve, there will be N reallocs, one for each resize call. Now it's reduced to log2(N), since with each reserve we double the buffer size. We still need to call resize to track the actual size.

@lidavidm

Copy link
Copy Markdown
Member

Ah, thanks for the explanation. I think it would be best if we could implement reserve for Java-allocated buffers, but I am not familiar with the code and don't have time to look at it.

@lidavidm

Copy link
Copy Markdown
Member

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

@js8544

Copy link
Copy Markdown
ContributorAuthor

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

It seems like we can. I can take a look later today if you don't have time for it.

@js8544js8544 changed the title ARROW-17869: [Java][Gandiva] Allow reserve not implemented in varlen output bufferARROW-17869: [Java][Gandiva] Implement Reserve for JavaBufferOct 4, 2022
@js8544js8544 closed this Oct 5, 2022
@pitrou

Copy link
Copy Markdown
Member

@js8544 Are you closing this PR in favor of another one?

@js8544

Copy link
Copy Markdown
ContributorAuthor

@js8544 Are you closing this PR in favor of another one?

@pitrou Yes, I will submit another PR once #14310 is merged since this change requires llvm@14.

pitrou pushed a commit that referenced this pull request Oct 11, 2022
See previous discussions at #14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@js8544
js8544 deleted the jinshang/allow_reserve_to_fail branch October 11, 2022 08:43
kou pushed a commit to apache/arrow-java that referenced this pull request Nov 25, 2024
See previous discussions at apache/arrow#14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@js8544@kou@pitrou@lidavidm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer - #14261

Closed
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail
Closed

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer#14261
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail

Conversation

@js8544

@js8544js8544 commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

Java's buffer type doesn't support reserve, which causes java-jars to fail.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has no components in JIRA, make sure you assign one.

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@github-actions

Copy link
Copy Markdown

Revision: 7f6d36cdbbf89f56f53e132198c86b0c004d119d

Submitted crossbow builds: ursacomputing/crossbow @ actions-d26b84437c

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

1 similar comment
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@kou Hi kou, sorry to bother. I requested two crossbow builds today but they were ignored by the bot. Do you have any idea why this happened?

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 263c2956ee7c09ab1e74fc5e5329e0c55e28d84c

Submitted crossbow builds: ursacomputing/crossbow @ actions-7ce585f8a8

TaskStatus
java-jarsGithub Actions

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

Hmm... It's strange...

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 1e7610799c2ee7724bb7a225bdfd2cafd0e811c5

Submitted crossbow builds: ursacomputing/crossbow @ actions-478721e321

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

Brew's LLVM 15 enables z3 by default, and it's shared linked. So at task Build Java Jars, it's required to have libz3 installed on the system. There are several ways to solve this:

  1. Add a brew install z3 task to Build Java Jars task
  2. Pin LLVM for java to 14. Currently it uses arrow/cpp/Brewfile. We can create another Brewfile in arrow/java and pin it in the new file.
  3. We can also pin LLVM to 14 for all MacOS builds at arrow/cpp/Brewfile. Because LLVM 15 is noticably slower than LLVM 14 somehow. It can also speed up the build time for other MacOS CI tasks.

Which way is preferred? cc @kou@pitrou

@kou

kou commented Sep 30, 2022

Copy link
Copy Markdown
Member
  1. for now but we need a new Jira issue for unpinning.

@js8544

Copy link
Copy Markdown
ContributorAuthor
  1. for now but we need a new Jira issue for unpinning.

I've pinned Brew llvm version to 14. Also filed https://issues.apache.org/jira/browse/ARROW-17902 to track current workarounds for LLVM 15 related issues.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 39437b059b466a00de6778034610600de6e15d0c

Submitted crossbow builds: ursacomputing/crossbow @ actions-e128cce1c4

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: dfb7e2d71450a3bff7191d2dda633c85d618b035

Submitted crossbow builds: ursacomputing/crossbow @ actions-77a61b58b9

TaskStatus
java-jarsGithub Actions

@js8544
js8544force-pushed the jinshang/allow_reserve_to_fail branch from dfb7e2d to 5710f8bCompareSeptember 30, 2022 08:10
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 5710f8b

Submitted crossbow builds: ursacomputing/crossbow @ actions-632d4e01ac

TaskStatus
java-jarsGithub Actions

@pitrou

Copy link
Copy Markdown
Member

Hmm... it would be much more desirable to implement Reserve on Java-exported buffers instead. @lwhite1@lidavidm Could one of you help perhaps?

@lidavidm

Copy link
Copy Markdown
Member

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

@js8544

Copy link
Copy Markdown
ContributorAuthor

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

Because the resize implementation for SimpleBuffer doesn't amotize the realloc costs, if we don't reserve, there will be N reallocs, one for each resize call. Now it's reduced to log2(N), since with each reserve we double the buffer size. We still need to call resize to track the actual size.

@lidavidm

Copy link
Copy Markdown
Member

Ah, thanks for the explanation. I think it would be best if we could implement reserve for Java-allocated buffers, but I am not familiar with the code and don't have time to look at it.

@lidavidm

Copy link
Copy Markdown
Member

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

@js8544

Copy link
Copy Markdown
ContributorAuthor

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

It seems like we can. I can take a look later today if you don't have time for it.

@js8544js8544 changed the title ARROW-17869: [Java][Gandiva] Allow reserve not implemented in varlen output bufferARROW-17869: [Java][Gandiva] Implement Reserve for JavaBufferOct 4, 2022
@js8544js8544 closed this Oct 5, 2022
@pitrou

Copy link
Copy Markdown
Member

@js8544 Are you closing this PR in favor of another one?

@js8544

Copy link
Copy Markdown
ContributorAuthor

@js8544 Are you closing this PR in favor of another one?

@pitrou Yes, I will submit another PR once #14310 is merged since this change requires llvm@14.

pitrou pushed a commit that referenced this pull request Oct 11, 2022
See previous discussions at #14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@js8544
js8544 deleted the jinshang/allow_reserve_to_fail branch October 11, 2022 08:43
kou pushed a commit to apache/arrow-java that referenced this pull request Nov 25, 2024
See previous discussions at apache/arrow#14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@js8544@kou@pitrou@lidavidm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer - #14261

Closed
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail
Closed

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer#14261
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail

Conversation

@js8544

@js8544js8544 commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

Java's buffer type doesn't support reserve, which causes java-jars to fail.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has no components in JIRA, make sure you assign one.

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@github-actions

Copy link
Copy Markdown

Revision: 7f6d36cdbbf89f56f53e132198c86b0c004d119d

Submitted crossbow builds: ursacomputing/crossbow @ actions-d26b84437c

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

1 similar comment
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@kou Hi kou, sorry to bother. I requested two crossbow builds today but they were ignored by the bot. Do you have any idea why this happened?

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 263c2956ee7c09ab1e74fc5e5329e0c55e28d84c

Submitted crossbow builds: ursacomputing/crossbow @ actions-7ce585f8a8

TaskStatus
java-jarsGithub Actions

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

Hmm... It's strange...

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 1e7610799c2ee7724bb7a225bdfd2cafd0e811c5

Submitted crossbow builds: ursacomputing/crossbow @ actions-478721e321

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

Brew's LLVM 15 enables z3 by default, and it's shared linked. So at task Build Java Jars, it's required to have libz3 installed on the system. There are several ways to solve this:

  1. Add a brew install z3 task to Build Java Jars task
  2. Pin LLVM for java to 14. Currently it uses arrow/cpp/Brewfile. We can create another Brewfile in arrow/java and pin it in the new file.
  3. We can also pin LLVM to 14 for all MacOS builds at arrow/cpp/Brewfile. Because LLVM 15 is noticably slower than LLVM 14 somehow. It can also speed up the build time for other MacOS CI tasks.

Which way is preferred? cc @kou@pitrou

@kou

kou commented Sep 30, 2022

Copy link
Copy Markdown
Member
  1. for now but we need a new Jira issue for unpinning.

@js8544

Copy link
Copy Markdown
ContributorAuthor
  1. for now but we need a new Jira issue for unpinning.

I've pinned Brew llvm version to 14. Also filed https://issues.apache.org/jira/browse/ARROW-17902 to track current workarounds for LLVM 15 related issues.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 39437b059b466a00de6778034610600de6e15d0c

Submitted crossbow builds: ursacomputing/crossbow @ actions-e128cce1c4

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: dfb7e2d71450a3bff7191d2dda633c85d618b035

Submitted crossbow builds: ursacomputing/crossbow @ actions-77a61b58b9

TaskStatus
java-jarsGithub Actions

@js8544
js8544force-pushed the jinshang/allow_reserve_to_fail branch from dfb7e2d to 5710f8bCompareSeptember 30, 2022 08:10
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 5710f8b

Submitted crossbow builds: ursacomputing/crossbow @ actions-632d4e01ac

TaskStatus
java-jarsGithub Actions

@pitrou

Copy link
Copy Markdown
Member

Hmm... it would be much more desirable to implement Reserve on Java-exported buffers instead. @lwhite1@lidavidm Could one of you help perhaps?

@lidavidm

Copy link
Copy Markdown
Member

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

@js8544

Copy link
Copy Markdown
ContributorAuthor

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

Because the resize implementation for SimpleBuffer doesn't amotize the realloc costs, if we don't reserve, there will be N reallocs, one for each resize call. Now it's reduced to log2(N), since with each reserve we double the buffer size. We still need to call resize to track the actual size.

@lidavidm

Copy link
Copy Markdown
Member

Ah, thanks for the explanation. I think it would be best if we could implement reserve for Java-allocated buffers, but I am not familiar with the code and don't have time to look at it.

@lidavidm

Copy link
Copy Markdown
Member

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

@js8544

Copy link
Copy Markdown
ContributorAuthor

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

It seems like we can. I can take a look later today if you don't have time for it.

@js8544js8544 changed the title ARROW-17869: [Java][Gandiva] Allow reserve not implemented in varlen output bufferARROW-17869: [Java][Gandiva] Implement Reserve for JavaBufferOct 4, 2022
@js8544js8544 closed this Oct 5, 2022
@pitrou

Copy link
Copy Markdown
Member

@js8544 Are you closing this PR in favor of another one?

@js8544

Copy link
Copy Markdown
ContributorAuthor

@js8544 Are you closing this PR in favor of another one?

@pitrou Yes, I will submit another PR once #14310 is merged since this change requires llvm@14.

pitrou pushed a commit that referenced this pull request Oct 11, 2022
See previous discussions at #14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@js8544
js8544 deleted the jinshang/allow_reserve_to_fail branch October 11, 2022 08:43
kou pushed a commit to apache/arrow-java that referenced this pull request Nov 25, 2024
See previous discussions at apache/arrow#14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@js8544@kou@pitrou@lidavidm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer - #14261

Closed
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail
Closed

ARROW-17869: [Java][Gandiva] Implement Reserve for JavaBuffer#14261
js8544 wants to merge 6 commits into
apache:masterfrom
js8544:jinshang/allow_reserve_to_fail

Conversation

@js8544

@js8544js8544 commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

Java's buffer type doesn't support reserve, which causes java-jars to fail.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has no components in JIRA, make sure you assign one.

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@github-actions

Copy link
Copy Markdown

Revision: 7f6d36cdbbf89f56f53e132198c86b0c004d119d

Submitted crossbow builds: ursacomputing/crossbow @ actions-d26b84437c

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

1 similar comment
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@kou Hi kou, sorry to bother. I requested two crossbow builds today but they were ignored by the bot. Do you have any idea why this happened?

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 263c2956ee7c09ab1e74fc5e5329e0c55e28d84c

Submitted crossbow builds: ursacomputing/crossbow @ actions-7ce585f8a8

TaskStatus
java-jarsGithub Actions

@kou

kou commented Sep 28, 2022

Copy link
Copy Markdown
Member

Hmm... It's strange...

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 1e7610799c2ee7724bb7a225bdfd2cafd0e811c5

Submitted crossbow builds: ursacomputing/crossbow @ actions-478721e321

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

Brew's LLVM 15 enables z3 by default, and it's shared linked. So at task Build Java Jars, it's required to have libz3 installed on the system. There are several ways to solve this:

  1. Add a brew install z3 task to Build Java Jars task
  2. Pin LLVM for java to 14. Currently it uses arrow/cpp/Brewfile. We can create another Brewfile in arrow/java and pin it in the new file.
  3. We can also pin LLVM to 14 for all MacOS builds at arrow/cpp/Brewfile. Because LLVM 15 is noticably slower than LLVM 14 somehow. It can also speed up the build time for other MacOS CI tasks.

Which way is preferred? cc @kou@pitrou

@kou

kou commented Sep 30, 2022

Copy link
Copy Markdown
Member
  1. for now but we need a new Jira issue for unpinning.

@js8544

Copy link
Copy Markdown
ContributorAuthor
  1. for now but we need a new Jira issue for unpinning.

I've pinned Brew llvm version to 14. Also filed https://issues.apache.org/jira/browse/ARROW-17902 to track current workarounds for LLVM 15 related issues.

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 39437b059b466a00de6778034610600de6e15d0c

Submitted crossbow builds: ursacomputing/crossbow @ actions-e128cce1c4

TaskStatus
java-jarsGithub Actions

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: dfb7e2d71450a3bff7191d2dda633c85d618b035

Submitted crossbow builds: ursacomputing/crossbow @ actions-77a61b58b9

TaskStatus
java-jarsGithub Actions

@js8544
js8544force-pushed the jinshang/allow_reserve_to_fail branch from dfb7e2d to 5710f8bCompareSeptember 30, 2022 08:10
@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@js8544

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit java-jars

@github-actions

Copy link
Copy Markdown

Revision: 5710f8b

Submitted crossbow builds: ursacomputing/crossbow @ actions-632d4e01ac

TaskStatus
java-jarsGithub Actions

@pitrou

Copy link
Copy Markdown
Member

Hmm... it would be much more desirable to implement Reserve on Java-exported buffers instead. @lwhite1@lidavidm Could one of you help perhaps?

@lidavidm

Copy link
Copy Markdown
Member

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

@js8544

Copy link
Copy Markdown
ContributorAuthor

From a quick glance only: why are we reserving if we're resizing immediately afterwards anyways?

Because the resize implementation for SimpleBuffer doesn't amotize the realloc costs, if we don't reserve, there will be N reallocs, one for each resize call. Now it's reduced to log2(N), since with each reserve we double the buffer size. We still need to call resize to track the actual size.

@lidavidm

Copy link
Copy Markdown
Member

Ah, thanks for the explanation. I think it would be best if we could implement reserve for Java-allocated buffers, but I am not familiar with the code and don't have time to look at it.

@lidavidm

Copy link
Copy Markdown
Member

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

@js8544

Copy link
Copy Markdown
ContributorAuthor

That said: it seems like we could recycle the logic for resize here to also implement reserve?

Status Reserve(constint64_t new_capacity) override {
returnStatus::NotImplemented("reserve not implemented");
}
private:
JNIEnv* env_;
jobject jexpander_;
int32_t vector_idx_;
};
Status JavaResizableBuffer::Resize(constint64_t new_size, bool shrink_to_fit) {
if (shrink_to_fit == true) {
returnStatus::NotImplemented("shrink not implemented");
}
if (ARROW_PREDICT_TRUE(new_size < capacity())) {
// no need to expand.
size_ = new_size;
returnStatus::OK();
}
// callback into java to expand the buffer
jobject ret =
env_->CallObjectMethod(jexpander_, vector_expander_method_, vector_idx_, new_size);
if (env_->ExceptionCheck()) {
env_->ExceptionDescribe();
env_->ExceptionClear();
returnStatus::OutOfMemory("buffer expand failed in java");
}
jlong ret_address = env_->GetLongField(ret, vector_expander_ret_address_);
jlong ret_capacity = env_->GetLongField(ret, vector_expander_ret_capacity_);
DCHECK_GE(ret_capacity, new_size);
data_ = reinterpret_cast<uint8_t*>(ret_address);
size_ = new_size;
capacity_ = ret_capacity;
returnStatus::OK();
}

It seems like we can. I can take a look later today if you don't have time for it.

@js8544js8544 changed the title ARROW-17869: [Java][Gandiva] Allow reserve not implemented in varlen output bufferARROW-17869: [Java][Gandiva] Implement Reserve for JavaBufferOct 4, 2022
@js8544js8544 closed this Oct 5, 2022
@pitrou

Copy link
Copy Markdown
Member

@js8544 Are you closing this PR in favor of another one?

@js8544

Copy link
Copy Markdown
ContributorAuthor

@js8544 Are you closing this PR in favor of another one?

@pitrou Yes, I will submit another PR once #14310 is merged since this change requires llvm@14.

pitrou pushed a commit that referenced this pull request Oct 11, 2022
See previous discussions at #14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@js8544
js8544 deleted the jinshang/allow_reserve_to_fail branch October 11, 2022 08:43
kou pushed a commit to apache/arrow-java that referenced this pull request Nov 25, 2024
See previous discussions at apache/arrow#14261
Authored-by: Jin Shang <shangjin1997@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@js8544@kou@pitrou@lidavidm